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

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

[Rayagonda Kokatanur]

[-- Attachment #1: Type: text/plain, Size: 649 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

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 v1 1/6] i2c: iproc: handle Master aborted error

[Rayagonda Kokatanur]

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

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

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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 --]

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

[Ray Jui]

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



On 10/11/2020 11:22 AM, Rayagonda Kokatanur wrote:
> Handle Master aborted error by flushing tx and rx fifo
> and reinitializing the hw.
> 
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@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);
> 

This looks fine to me. Thanks.

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



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

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

[Rayagonda Kokatanur]

[-- Attachment #1: Type: text/plain, Size: 1554 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>
---
 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 --]

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

[Ray Jui]

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



On 10/11/2020 11:22 AM, Rayagonda Kokatanur wrote:
> 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>
> ---
>  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
> 

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

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

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

[Rayagonda Kokatanur]

[-- Attachment #1: Type: text/plain, Size: 973 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>
---
 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 --]

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

[Ray Jui]

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



On 10/11/2020 11:22 AM, Rayagonda Kokatanur wrote:
> 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>
> ---
>  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);
> 

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

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

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

[Rayagonda Kokatanur]

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

Fix typo in bcm_iproc_i2c_slave_isr().

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

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

[Ray Jui]

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



On 10/11/2020 11:22 AM, Rayagonda Kokatanur wrote:
> Fix typo in bcm_iproc_i2c_slave_isr().
> 
> Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")

This is merely a fix of typo in code comment and there's no functional
impact. Why do we need a Fixes tag on this (which indicates the fix
needs to be backported to LTS kernels)?

> 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);
> 

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

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

[Rayagonda Kokatanur]

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

On Fri, Oct 23, 2020 at 10:51 PM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 10/11/2020 11:22 AM, Rayagonda Kokatanur wrote:
> > Fix typo in bcm_iproc_i2c_slave_isr().
> >
> > Fixes: c245d94ed106 ("i2c: iproc: Add multi byte read-write support for slave mode")
>
> This is merely a fix of typo in code comment and there's no functional
> impact. Why do we need a Fixes tag on this (which indicates the fix
> needs to be backported to LTS kernels)?

Thank you Ray.
I will remove Fixes tag and push patch v2.

Best regards,
Rayagonda

>
> > 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);
> >

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

[PATCH v1 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 v1 5/6] i2c: iproc: handle master read request

[Dhananjay Phadke]

On Sun, 11 Oct 2020 23:52:53 +0530, Rayagonda Kokatanur wrote:
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> 
> -		} 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++;

rx_bytes should be incremented only along with I2C_SLAVE_WRITE_RECEIVED event?

> 
> +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));
> 

Both tasklet and isr are writing to status (IS_OFFSET) reg.

The tasklet seems to be batching up rx fifo reads because of time-sensitive
Master-write-read transaction? Linux I2C framework is byte interface anyway.
Can the need to batch reads be avoided by setting slave rx threshold for
interrupt (S_FIFO_RX_THLD) to 1-byte? 

Also, wouldn't tasklets be susceptible to other interrupts? If fifo reads
have to be batched up, can it be changed to threaded irq?

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

[Rayagonda Kokatanur]

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

On Wed, Oct 14, 2020 at 8:50 AM Dhananjay Phadke
<dphadke@linux.microsoft.com> wrote:
>
> On Sun, 11 Oct 2020 23:52:53 +0530, Rayagonda Kokatanur wrote:
> > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> >
> > -             } 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++;
>
> rx_bytes should be incremented only along with I2C_SLAVE_WRITE_RECEIVED event?

It should be incremented in both I2C_SLAVE_WRITE_REQUESTED and
I2C_SLAVE_WRITE_RECEIVED cases because in both cases it is reading
valid bytes from rx fifo.

>
> >
> > +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));
> >
>
> Both tasklet and isr are writing to status (IS_OFFSET) reg.
>
> The tasklet seems to be batching up rx fifo reads because of time-sensitive
> Master-write-read transaction? Linux I2C framework is byte interface anyway.
> Can the need to batch reads be avoided by setting slave rx threshold for
> interrupt (S_FIFO_RX_THLD) to 1-byte?

To process more data with a single interrupt we are batching up rx fifo reads.
This will reduce the number of interrupts.

Also to avoid tasklet running more time (20us) we have a threshold of
10 bytes for batching read.
This is a better/optimised approach than reading single byte data per interrupt.

>
> Also, wouldn't tasklets be susceptible to other interrupts? If fifo reads
> have to be batched up, can it be changed to threaded irq?

tasklets have higher priority than threaded irq, since i2c is time
sensitive so using a tasklet is preferred over threaded irq.

Best regards,
Rayagonda

>
>

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

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

[Ray Jui]

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



On 10/13/2020 10:12 PM, Rayagonda Kokatanur wrote:
> 
> 
> On Wed, Oct 14, 2020 at 8:50 AM Dhananjay Phadke
> <dphadke@linux.microsoft.com <mailto:dphadke@linux.microsoft.com>> wrote:
> 
>     On Sun, 11 Oct 2020 23:52:53 +0530, Rayagonda Kokatanur wrote:
>     > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
>     > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
>     >
>     > -             } 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++;
> 
>     rx_bytes should be incremented only along with
>     I2C_SLAVE_WRITE_RECEIVED event?
> 
> 
> It should be incremented in both I2C_SLAVE_WRITE_REQUESTED and  
> I2C_SLAVE_WRITE_RECEIVED cases because in both case it is reading valid
> bytes from rx fifo.
> 
> 
>     >
>     > +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));
>     >
> 
>     Both tasklet and isr are writing to status (IS_OFFSET) reg.
> 
> 
> Yes this is required.
> 
> For ex, If IS_S_RD_EVENT_SHIFT interrupt, this should be cleared once
> the driver completes reading all data from rx fifo.
> After this the driver can start sending data to master.
>  

If both tasklet and isr are accessing the IS_OFFSET register, don't you
need lock protection against race condition? That is, ISR can interrupt
tasklet.

> 
> 
>     The tasklet seems to be batching up rx fifo reads because of
>     time-sensitive
>     Master-write-read transaction? Linux I2C framework is byte interface
>     anyway.
>     Can the need to batch reads be avoided by setting slave rx threshold for
>     interrupt (S_FIFO_RX_THLD) to 1-byte?
> 
> 
> To process more data with a single interrupt we are batching up rx fifo
> reads.
> This will reduce the number of interrupts.
> 
> Also to avoid tasklet running more time (20us) we have a threshold of 10
> bytes for batching read.
> This is a better/optimised approach than reading single byte data per
> interrupt.
> 
> 
>     Also, wouldn't tasklets be susceptible to other interrupts? If fifo
>     reads
>     have to be batched up, can it be changed to threaded irq?
> 
> 
> tasklets have higher priority than threaded irq, since i2c is time
> sensitive so using a tasklet is preferred over threaded irq.
>  

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

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

[Rayagonda Kokatanur]

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

On Fri, Oct 23, 2020 at 10:56 PM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 10/13/2020 10:12 PM, Rayagonda Kokatanur wrote:
> >
> >
> > On Wed, Oct 14, 2020 at 8:50 AM Dhananjay Phadke
> > <dphadke@linux.microsoft.com <mailto:dphadke@linux.microsoft.com>> wrote:
> >
> >     On Sun, 11 Oct 2020 23:52:53 +0530, Rayagonda Kokatanur wrote:
> >     > --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> >     > +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> >     >
> >     > -             } 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++;
> >
> >     rx_bytes should be incremented only along with
> >     I2C_SLAVE_WRITE_RECEIVED event?
> >
> >
> > It should be incremented in both I2C_SLAVE_WRITE_REQUESTED and
> > I2C_SLAVE_WRITE_RECEIVED cases because in both case it is reading valid
> > bytes from rx fifo.
> >
> >
> >     >
> >     > +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));
> >     >
> >
> >     Both tasklet and isr are writing to status (IS_OFFSET) reg.
> >
> >
> > Yes this is required.
> >
> > For ex, If IS_S_RD_EVENT_SHIFT interrupt, this should be cleared once
> > the driver completes reading all data from rx fifo.
> > After this the driver can start sending data to master.
> >
>
> If both tasklet and isr are accessing the IS_OFFSET register, don't you
> need lock protection against race condition? That is, ISR can interrupt
> tasklet.

All interrupts are disbaled when the tasklet is running.
Interrupts are re-enabled at the end of the tasklet.
So no race condition between tasklet and isr.

Best regards,
Rayagonda

>
> >
> >
> >     The tasklet seems to be batching up rx fifo reads because of
> >     time-sensitive
> >     Master-write-read transaction? Linux I2C framework is byte interface
> >     anyway.
> >     Can the need to batch reads be avoided by setting slave rx threshold for
> >     interrupt (S_FIFO_RX_THLD) to 1-byte?
> >
> >
> > To process more data with a single interrupt we are batching up rx fifo
> > reads.
> > This will reduce the number of interrupts.
> >
> > Also to avoid tasklet running more time (20us) we have a threshold of 10
> > bytes for batching read.
> > This is a better/optimised approach than reading single byte data per
> > interrupt.
> >
> >
> >     Also, wouldn't tasklets be susceptible to other interrupts? If fifo
> >     reads
> >     have to be batched up, can it be changed to threaded irq?
> >
> >
> > tasklets have higher priority than threaded irq, since i2c is time
> > sensitive so using a tasklet is preferred over threaded irq.
> >

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

[PATCH v1 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 v1 6/6] i2c: iproc: handle rx fifo full interrupt

[Dhananjay Phadke]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

On Sun, 11 Oct 2020 23:52:54 +0530, Rayagonda Kokatanur wrote:
> 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.
> 

ARM cores run much faster than I2C bus, why would rx fifo go full when
rx interrupt is enabled and bytes are read out by bus driver isr?
Isn't fifo read pointer updated on these byte reads?
Does controller stretch clock when rx fifo is full (e.g. kernel has
crashed, bus driver isn't draining fifo)?

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

[Ray Jui]

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



On 10/12/2020 3:03 PM, Dhananjay Phadke wrote:
> From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> 
> On Sun, 11 Oct 2020 23:52:54 +0530, Rayagonda Kokatanur wrote:
>> 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.
>>
> 
> ARM cores run much faster than I2C bus, why would rx fifo go full when
> rx interrupt is enabled and bytes are read out by bus driver isr?
> Isn't fifo read pointer updated on these byte reads?

Hi Rayagonda,

Could you please reply on this question? For transactions > 64 bytes, do
we batch until RX FIFO is full before we read out the data?

Thanks,

Ray

> Does controller stretch clock when rx fifo is full (e.g. kernel has
> crashed, bus driver isn't draining fifo)?
> 

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

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

[Rayagonda Kokatanur]

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

Hi Dhanajay,

On Fri, Oct 23, 2020 at 11:12 PM Ray Jui <ray.jui@broadcom.com> wrote:
>
>
>
> On 10/12/2020 3:03 PM, Dhananjay Phadke wrote:
> > From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> >
> > On Sun, 11 Oct 2020 23:52:54 +0530, Rayagonda Kokatanur wrote:
> >> 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.
> >>
> >
> > ARM cores run much faster than I2C bus, why would rx fifo go full when
> > rx interrupt is enabled and bytes are read out by bus driver isr?
> > Isn't fifo read pointer updated on these byte reads?
>
> Hi Rayagonda,
>
> Could you please reply on this question? For transactions > 64 bytes, do
> we batch until RX FIFO is full before we read out the data?

Sorry I missed this question.
Yes with current design we are batching 64 bytes for translation > 64 bytes.

Best regards,
Rayagonda


>
> Thanks,
>
> Ray
>
> > Does controller stretch clock when rx fifo is full (e.g. kernel has
> > crashed, bus driver isn't draining fifo)?
> >

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

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

[Ray Jui]

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



On 10/26/2020 8:13 AM, Rayagonda Kokatanur wrote:
> Hi Dhanajay,
> 
> On Fri, Oct 23, 2020 at 11:12 PM Ray Jui <ray.jui@broadcom.com> wrote:
>>
>>
>>
>> On 10/12/2020 3:03 PM, Dhananjay Phadke wrote:
>>> From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>>>
>>> On Sun, 11 Oct 2020 23:52:54 +0530, Rayagonda Kokatanur wrote:
>>>> 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.
>>>>
>>>
>>> ARM cores run much faster than I2C bus, why would rx fifo go full when
>>> rx interrupt is enabled and bytes are read out by bus driver isr?
>>> Isn't fifo read pointer updated on these byte reads?
>>
>> Hi Rayagonda,
>>
>> Could you please reply on this question? For transactions > 64 bytes, do
>> we batch until RX FIFO is full before we read out the data?
> 
> Sorry I missed this question.
> Yes with current design we are batching 64 bytes for translation > 64 bytes.
> 

So we do batch the transfer and read them in one shot, and that's how
the FIFO full interrupt is being utilized for. That sounds okay to me.

Thanks,

Ray

> Best regards,
> Rayagonda
> 
> 
>>
>> Thanks,
>>
>> Ray
>>
>>> Does controller stretch clock when rx fifo is full (e.g. kernel has
>>> crashed, bus driver isn't draining fifo)?
>>>

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