[PATCH 0/3] i2c-axxia: support Sequence command

[PATCH 0/3] i2c-axxia: support Sequence command

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

Hi Woflram,

This patch was based on your i2c/for-current branch as it adds to my
patch you incuded there. First two patches are just preparations and the
3rd one which is the actual change. It allows us using hardware mode to
send i2c transfers consisting for a write followed by read without the
need of the CPU interaction inbetween which is an opportunity to trigger
25ms SMbus timeout and abort transaction.

--------------------------------------------------------------------
Krzysztof Adamski (3):
  i2c-axxia: dedicated function to set client addr
  i2c-axxia: check for error conditions first
  i2c-axxia: support sequence command mode

 drivers/i2c/busses/i2c-axxia.c | 159 ++++++++++++++++++++++++++-------
 1 file changed, 126 insertions(+), 33 deletions(-)

-- 
2.17.2

[PATCH 1/3] i2c-axxia: dedicated function to set client addr

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

This patch moves configuration of hardware registers used for setting
i2c client address to separate function. It is preparatory change for
next commit.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/i2c/busses/i2c-axxia.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index 51d34959709b..d37caf694fbf 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -337,17 +337,9 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 	return IRQ_HANDLED;
 }
 
-static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
+static void axxia_i2c_set_addr(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 {
-	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
-	u32 rx_xfer, tx_xfer;
 	u32 addr_1, addr_2;
-	unsigned long time_left;
-	unsigned int wt_value;
-
-	idev->msg = msg;
-	idev->msg_xfrd = 0;
-	reinit_completion(&idev->msg_complete);
 
 	if (i2c_m_ten(msg)) {
 		/* 10-bit address
@@ -367,6 +359,23 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 		addr_2 = 0;
 	}
 
+	writel(addr_1, idev->base + MST_ADDR_1);
+	writel(addr_2, idev->base + MST_ADDR_2);
+}
+
+static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
+{
+	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
+	u32 rx_xfer, tx_xfer;
+	unsigned long time_left;
+	unsigned int wt_value;
+
+	idev->msg = msg;
+	idev->msg_xfrd = 0;
+	reinit_completion(&idev->msg_complete);
+
+	axxia_i2c_set_addr(idev, msg);
+
 	if (i2c_m_rd(msg)) {
 		/* I2C read transfer */
 		rx_xfer = i2c_m_recv_len(msg) ? I2C_SMBUS_BLOCK_MAX : msg->len;
@@ -379,8 +388,6 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 
 	writel(rx_xfer, idev->base + MST_RX_XFER);
 	writel(tx_xfer, idev->base + MST_TX_XFER);
-	writel(addr_1, idev->base + MST_ADDR_1);
-	writel(addr_2, idev->base + MST_ADDR_2);
 
 	if (i2c_m_rd(msg))
 		int_mask |= MST_STATUS_RFL;
-- 
2.17.2

Re: [PATCH 1/3] i2c-axxia: dedicated function to set client addr

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi!

On 10/12/2018 16:00, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> This patch moves configuration of hardware registers used for setting
> i2c client address to separate function. It is preparatory change for
> next commit.

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>  drivers/i2c/busses/i2c-axxia.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
> index 51d34959709b..d37caf694fbf 100644
> --- a/drivers/i2c/busses/i2c-axxia.c
> +++ b/drivers/i2c/busses/i2c-axxia.c
> @@ -337,17 +337,9 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  	return IRQ_HANDLED;
>  }
>  
> -static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
> +static void axxia_i2c_set_addr(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  {
> -	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
> -	u32 rx_xfer, tx_xfer;
>  	u32 addr_1, addr_2;
> -	unsigned long time_left;
> -	unsigned int wt_value;
> -
> -	idev->msg = msg;
> -	idev->msg_xfrd = 0;
> -	reinit_completion(&idev->msg_complete);
>  
>  	if (i2c_m_ten(msg)) {
>  		/* 10-bit address
> @@ -367,6 +359,23 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  		addr_2 = 0;
>  	}
>  
> +	writel(addr_1, idev->base + MST_ADDR_1);
> +	writel(addr_2, idev->base + MST_ADDR_2);
> +}
> +
> +static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
> +{
> +	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
> +	u32 rx_xfer, tx_xfer;
> +	unsigned long time_left;
> +	unsigned int wt_value;
> +
> +	idev->msg = msg;
> +	idev->msg_xfrd = 0;
> +	reinit_completion(&idev->msg_complete);
> +
> +	axxia_i2c_set_addr(idev, msg);
> +
>  	if (i2c_m_rd(msg)) {
>  		/* I2C read transfer */
>  		rx_xfer = i2c_m_recv_len(msg) ? I2C_SMBUS_BLOCK_MAX : msg->len;
> @@ -379,8 +388,6 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  
>  	writel(rx_xfer, idev->base + MST_RX_XFER);
>  	writel(tx_xfer, idev->base + MST_TX_XFER);
> -	writel(addr_1, idev->base + MST_ADDR_1);
> -	writel(addr_2, idev->base + MST_ADDR_2);
>  
>  	if (i2c_m_rd(msg))
>  		int_mask |= MST_STATUS_RFL;

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 1/3] i2c-axxia: dedicated function to set client addr

[Wolfram Sang]

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

On Mon, Dec 10, 2018 at 03:00:52PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> This patch moves configuration of hardware registers used for setting
> i2c client address to separate function. It is preparatory change for
> next commit.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Applied to for-next, thanks!


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

[PATCH 2/3] i2c-axxia: check for error conditions first

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

It was observed that when using seqentional mode contrary to the
documentation, the SS bit (which is supposed to only be set if
automatic/sequence command completed normally), is sometimes set
together with NA (NAK in address phase) causing transfer to falsely be
considered successful.

My assumption is that this does not happen during manual mode since the
controller is stopping its work the moment it sets NA/ND bit in status
register. This is not the case in Automatic/Sequentional mode where it
is still working to send STOP condition and the actual status we get
depends on the time when the ISR is run.

This patch changes the order of checking status bits in ISR - error
conditions are checked first and only if none of them occurred, the
transfer may be considered successful. This is required to introduce
using of sequentional mode in next patch.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/i2c/busses/i2c-axxia.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index d37caf694fbf..35258321e81b 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -296,22 +296,7 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 			i2c_int_disable(idev, MST_STATUS_TFL);
 	}
 
-	if (status & MST_STATUS_SCC) {
-		/* Stop completed */
-		i2c_int_disable(idev, ~MST_STATUS_TSS);
-		complete(&idev->msg_complete);
-	} else if (status & MST_STATUS_SNS) {
-		/* Transfer done */
-		i2c_int_disable(idev, ~MST_STATUS_TSS);
-		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
-			axxia_i2c_empty_rx_fifo(idev);
-		complete(&idev->msg_complete);
-	} else if (status & MST_STATUS_TSS) {
-		/* Transfer timeout */
-		idev->msg_err = -ETIMEDOUT;
-		i2c_int_disable(idev, ~MST_STATUS_TSS);
-		complete(&idev->msg_complete);
-	} else if (unlikely(status & MST_STATUS_ERR)) {
+	if (unlikely(status & MST_STATUS_ERR)) {
 		/* Transfer error */
 		i2c_int_disable(idev, ~0);
 		if (status & MST_STATUS_AL)
@@ -328,6 +313,21 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 			readl(idev->base + MST_TX_BYTES_XFRD),
 			readl(idev->base + MST_TX_XFER));
 		complete(&idev->msg_complete);
+	} else if (status & MST_STATUS_SCC) {
+		/* Stop completed */
+		i2c_int_disable(idev, ~MST_STATUS_TSS);
+		complete(&idev->msg_complete);
+	} else if (status & MST_STATUS_SNS) {
+		/* Transfer done */
+		i2c_int_disable(idev, ~MST_STATUS_TSS);
+		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
+			axxia_i2c_empty_rx_fifo(idev);
+		complete(&idev->msg_complete);
+	} else if (status & MST_STATUS_TSS) {
+		/* Transfer timeout */
+		idev->msg_err = -ETIMEDOUT;
+		i2c_int_disable(idev, ~MST_STATUS_TSS);
+		complete(&idev->msg_complete);
 	}
 
 out:
-- 
2.17.2

Re: [PATCH 2/3] i2c-axxia: check for error conditions first

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi!

On 10/12/2018 16:01, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> It was observed that when using seqentional mode contrary to the
                                  ^^^^^^^^^^^
> documentation, the SS bit (which is supposed to only be set if
> automatic/sequence command completed normally), is sometimes set
> together with NA (NAK in address phase) causing transfer to falsely be
> considered successful.
> 
> My assumption is that this does not happen during manual mode since the
> controller is stopping its work the moment it sets NA/ND bit in status
> register. This is not the case in Automatic/Sequentional mode where it
                                              ^^^^^^^^^^^^
> is still working to send STOP condition and the actual status we get
> depends on the time when the ISR is run.
> 
> This patch changes the order of checking status bits in ISR - error
> conditions are checked first and only if none of them occurred, the
> transfer may be considered successful. This is required to introduce
> using of sequentional mode in next patch.
           ^^^^^^^^^^^^
sequential

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>  drivers/i2c/busses/i2c-axxia.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
> index d37caf694fbf..35258321e81b 100644
> --- a/drivers/i2c/busses/i2c-axxia.c
> +++ b/drivers/i2c/busses/i2c-axxia.c
> @@ -296,22 +296,7 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  			i2c_int_disable(idev, MST_STATUS_TFL);
>  	}
>  
> -	if (status & MST_STATUS_SCC) {
> -		/* Stop completed */
> -		i2c_int_disable(idev, ~MST_STATUS_TSS);
> -		complete(&idev->msg_complete);
> -	} else if (status & MST_STATUS_SNS) {
> -		/* Transfer done */
> -		i2c_int_disable(idev, ~MST_STATUS_TSS);
> -		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
> -			axxia_i2c_empty_rx_fifo(idev);
> -		complete(&idev->msg_complete);
> -	} else if (status & MST_STATUS_TSS) {
> -		/* Transfer timeout */
> -		idev->msg_err = -ETIMEDOUT;
> -		i2c_int_disable(idev, ~MST_STATUS_TSS);
> -		complete(&idev->msg_complete);
> -	} else if (unlikely(status & MST_STATUS_ERR)) {
> +	if (unlikely(status & MST_STATUS_ERR)) {
>  		/* Transfer error */
>  		i2c_int_disable(idev, ~0);
>  		if (status & MST_STATUS_AL)
> @@ -328,6 +313,21 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  			readl(idev->base + MST_TX_BYTES_XFRD),
>  			readl(idev->base + MST_TX_XFER));
>  		complete(&idev->msg_complete);
> +	} else if (status & MST_STATUS_SCC) {
> +		/* Stop completed */
> +		i2c_int_disable(idev, ~MST_STATUS_TSS);
> +		complete(&idev->msg_complete);
> +	} else if (status & MST_STATUS_SNS) {
> +		/* Transfer done */
> +		i2c_int_disable(idev, ~MST_STATUS_TSS);
> +		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
> +			axxia_i2c_empty_rx_fifo(idev);
> +		complete(&idev->msg_complete);
> +	} else if (status & MST_STATUS_TSS) {
> +		/* Transfer timeout */
> +		idev->msg_err = -ETIMEDOUT;
> +		i2c_int_disable(idev, ~MST_STATUS_TSS);
> +		complete(&idev->msg_complete);
>  	}
>  
>  out:
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 2/3] i2c-axxia: check for error conditions first

[Wolfram Sang]

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

On Mon, Dec 10, 2018 at 03:01:27PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> It was observed that when using seqentional mode contrary to the
> documentation, the SS bit (which is supposed to only be set if
> automatic/sequence command completed normally), is sometimes set
> together with NA (NAK in address phase) causing transfer to falsely be
> considered successful.
> 
> My assumption is that this does not happen during manual mode since the
> controller is stopping its work the moment it sets NA/ND bit in status
> register. This is not the case in Automatic/Sequentional mode where it
> is still working to send STOP condition and the actual status we get
> depends on the time when the ISR is run.
> 
> This patch changes the order of checking status bits in ISR - error
> conditions are checked first and only if none of them occurred, the
> transfer may be considered successful. This is required to introduce
> using of sequentional mode in next patch.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Applied to for-next, thanks!


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

[PATCH 3/3] i2c-axxia: support sequence command mode

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

In order to comply with SMBus specification, the Axxia I²C module will
abort the multi message transfer if the delay between finishing sending
one message and starting another is longer than 25ms. Unfortunately it
isn't that hard to trigger this situation on a busy system. In order to
fix this problem, we should make sure hardware does whole transaction
without waiting for software to fill some data.

Fortunately, in addition to Manual mode that is currently used by the
driver to perform I²C transfers, the module supports also so called
Sequence mode. In this mode, the module automatically performs
predefined sequence of operations - it sends a slave address, transmits
specified number of bytes from the FIFO, changes transfer direction,
resends the slave address and then reads specified number of bytes to
FIFO. While very inflexible, this does fit a most common case of multi
message transfer - the one where you first write a register number you
want to read and then read it.

To use this mode effectively, a number of conditions must be met to
ensure the transaction does fit the predefined sequence. In case this is
not the case, a fallback to manual mode is used.

The initialization of this mode is very similar to Manual mode. The most
notable difference is different bit in the Master Interrupt Status
designating finishing of transaction. Also some of the errors, like TSS,
cannot happen in this mode.

While it is possible to support transactions requesting a read of any
size (RFL interrupt will be generated when FIFO size is not enough) the
TFL interrupt is not available in this mode, thus the write part of the
transaction cannot exceed FIFO_SIZE (8).

Note that in case of a NAK during transaction, the NA/ND status bits
will be set before STOP command is generated, triggering an interrupt
while the controller is still busy. Current solution for this problem is
to actively wait for this command to stop before leaving xfer callback.

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
---
 drivers/i2c/busses/i2c-axxia.c | 100 ++++++++++++++++++++++++++++++---
 1 file changed, 93 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index 35258321e81b..64779819a948 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -12,6 +12,7 @@
  */
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
@@ -25,6 +26,7 @@
 #define I2C_XFER_TIMEOUT    (msecs_to_jiffies(250))
 #define I2C_STOP_TIMEOUT    (msecs_to_jiffies(100))
 #define FIFO_SIZE           8
+#define SEQ_LEN             2
 
 #define GLOBAL_CONTROL		0x00
 #define   GLOBAL_MST_EN         BIT(0)
@@ -51,6 +53,7 @@
 #define   CMD_BUSY		(1<<3)
 #define   CMD_MANUAL		(0x00 | CMD_BUSY)
 #define   CMD_AUTO		(0x01 | CMD_BUSY)
+#define   CMD_SEQUENCE		(0x02 | CMD_BUSY)
 #define MST_RX_XFER		0x2c
 #define MST_TX_XFER		0x30
 #define MST_ADDR_1		0x34
@@ -87,7 +90,9 @@
  * axxia_i2c_dev - I2C device context
  * @base: pointer to register struct
  * @msg: pointer to current message
- * @msg_xfrd: number of bytes transferred in msg
+ * @msg_r: pointer to current read message (sequence transfer)
+ * @msg_xfrd: number of bytes transferred in tx_fifo
+ * @msg_xfrd_r: number of bytes transferred in rx_fifo
  * @msg_err: error code for completed message
  * @msg_complete: xfer completion object
  * @dev: device reference
@@ -98,7 +103,9 @@
 struct axxia_i2c_dev {
 	void __iomem *base;
 	struct i2c_msg *msg;
+	struct i2c_msg *msg_r;
 	size_t msg_xfrd;
+	size_t msg_xfrd_r;
 	int msg_err;
 	struct completion msg_complete;
 	struct device *dev;
@@ -227,14 +234,14 @@ static int i2c_m_recv_len(const struct i2c_msg *msg)
  */
 static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
 {
-	struct i2c_msg *msg = idev->msg;
+	struct i2c_msg *msg = idev->msg_r;
 	size_t rx_fifo_avail = readl(idev->base + MST_RX_FIFO);
-	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd);
+	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd_r);
 
 	while (bytes_to_transfer-- > 0) {
 		int c = readl(idev->base + MST_DATA);
 
-		if (idev->msg_xfrd == 0 && i2c_m_recv_len(msg)) {
+		if (idev->msg_xfrd_r == 0 && i2c_m_recv_len(msg)) {
 			/*
 			 * Check length byte for SMBus block read
 			 */
@@ -247,7 +254,7 @@ static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
 			msg->len = 1 + c;
 			writel(msg->len, idev->base + MST_RX_XFER);
 		}
-		msg->buf[idev->msg_xfrd++] = c;
+		msg->buf[idev->msg_xfrd_r++] = c;
 	}
 
 	return 0;
@@ -287,7 +294,7 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 	}
 
 	/* RX FIFO needs service? */
-	if (i2c_m_rd(idev->msg) && (status & MST_STATUS_RFL))
+	if (i2c_m_rd(idev->msg_r) && (status & MST_STATUS_RFL))
 		axxia_i2c_empty_rx_fifo(idev);
 
 	/* TX FIFO needs service? */
@@ -320,9 +327,12 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 	} else if (status & MST_STATUS_SNS) {
 		/* Transfer done */
 		i2c_int_disable(idev, ~MST_STATUS_TSS);
-		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
+		if (i2c_m_rd(idev->msg_r) && idev->msg_xfrd_r < idev->msg_r->len)
 			axxia_i2c_empty_rx_fifo(idev);
 		complete(&idev->msg_complete);
+	} else if (status & MST_STATUS_SS) {
+		/* Auto/Sequence transfer done */
+		complete(&idev->msg_complete);
 	} else if (status & MST_STATUS_TSS) {
 		/* Transfer timeout */
 		idev->msg_err = -ETIMEDOUT;
@@ -363,6 +373,62 @@ static void axxia_i2c_set_addr(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	writel(addr_2, idev->base + MST_ADDR_2);
 }
 
+/* The NAK interrupt will be sent _before_ issuing STOP command
+ * so the controller might still be busy processing it. No
+ * interrupt will be sent at the end so we have to poll for it
+ */
+static void axxia_i2c_handle_seq_nak(struct axxia_i2c_dev *idev)
+{
+	while (readl(idev->base + MST_COMMAND) & CMD_BUSY)
+		udelay(100);
+}
+
+static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
+{
+	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SS | MST_STATUS_RFL;
+	u32 rlen = i2c_m_recv_len(&msgs[1]) ? I2C_SMBUS_BLOCK_MAX : msgs[1].len;
+	unsigned long time_left;
+
+	axxia_i2c_set_addr(idev, &msgs[0]);
+
+	writel(msgs[0].len, idev->base + MST_TX_XFER);
+	writel(rlen, idev->base + MST_RX_XFER);
+
+	idev->msg = &msgs[0];
+	idev->msg_r = &msgs[1];
+	idev->msg_xfrd = 0;
+	idev->msg_xfrd_r = 0;
+	axxia_i2c_fill_tx_fifo(idev);
+
+	writel(CMD_SEQUENCE, idev->base + MST_COMMAND);
+
+	reinit_completion(&idev->msg_complete);
+	i2c_int_enable(idev, int_mask);
+
+	time_left = wait_for_completion_timeout(&idev->msg_complete,
+						I2C_XFER_TIMEOUT);
+
+	i2c_int_disable(idev, int_mask);
+
+	axxia_i2c_empty_rx_fifo(idev);
+
+	if (idev->msg_err == -ENXIO)
+		axxia_i2c_handle_seq_nak(idev);
+	else if (readl(idev->base + MST_COMMAND) & CMD_BUSY)
+		dev_warn(idev->dev, "busy after xfer\n");
+
+	if (time_left == 0) {
+		idev->msg_err = -ETIMEDOUT;
+		i2c_recover_bus(&idev->adapter);
+		axxia_i2c_init(idev);
+	}
+
+	if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
+		axxia_i2c_init(idev);
+
+	return idev->msg_err;
+}
+
 static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 {
 	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
@@ -371,7 +437,9 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	unsigned int wt_value;
 
 	idev->msg = msg;
+	idev->msg_r = msg;
 	idev->msg_xfrd = 0;
+	idev->msg_xfrd_r = 0;
 	reinit_completion(&idev->msg_complete);
 
 	axxia_i2c_set_addr(idev, msg);
@@ -452,6 +520,18 @@ static int axxia_i2c_stop(struct axxia_i2c_dev *idev)
 	return 0;
 }
 
+/* This function checks if the msgs[] array contains messages compatible with
+ * Sequence mode of operation. This mode assumes there will be exactly one
+ * write of non-zero length followed by exactly one read of non-zero length,
+ * both targeted at the same client device.
+ */
+static bool axxia_i2c_sequence_ok(struct i2c_msg msgs[], int num)
+{
+	return num == SEQ_LEN && !i2c_m_rd(&msgs[0]) && i2c_m_rd(&msgs[1]) &&
+	       msgs[0].len > 0 && msgs[0].len <= FIFO_SIZE &&
+	       msgs[1].len > 0 && msgs[0].addr == msgs[1].addr;
+}
+
 static int
 axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
@@ -460,6 +540,12 @@ axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int ret = 0;
 
 	idev->msg_err = 0;
+
+	if (axxia_i2c_sequence_ok(msgs, num)) {
+		ret = axxia_i2c_xfer_seq(idev, msgs);
+		return ret ? : SEQ_LEN;
+	}
+
 	i2c_int_enable(idev, MST_STATUS_TSS);
 
 	for (i = 0; ret == 0 && i < num; ++i)
-- 
2.17.2

Re: [PATCH 3/3] i2c-axxia: support sequence command mode

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi!

On 10/12/2018 16:05, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> In order to comply with SMBus specification, the Axxia I²C module will
> abort the multi message transfer if the delay between finishing sending
> one message and starting another is longer than 25ms. Unfortunately it
> isn't that hard to trigger this situation on a busy system. In order to
> fix this problem, we should make sure hardware does whole transaction
> without waiting for software to fill some data.
> 
> Fortunately, in addition to Manual mode that is currently used by the
> driver to perform I²C transfers, the module supports also so called
> Sequence mode. In this mode, the module automatically performs
> predefined sequence of operations - it sends a slave address, transmits
> specified number of bytes from the FIFO, changes transfer direction,
> resends the slave address and then reads specified number of bytes to
> FIFO. While very inflexible, this does fit a most common case of multi
> message transfer - the one where you first write a register number you
> want to read and then read it.
> 
> To use this mode effectively, a number of conditions must be met to
> ensure the transaction does fit the predefined sequence. In case this is
> not the case, a fallback to manual mode is used.
> 
> The initialization of this mode is very similar to Manual mode. The most
> notable difference is different bit in the Master Interrupt Status
> designating finishing of transaction. Also some of the errors, like TSS,
> cannot happen in this mode.
> 
> While it is possible to support transactions requesting a read of any
> size (RFL interrupt will be generated when FIFO size is not enough) the
> TFL interrupt is not available in this mode, thus the write part of the
> transaction cannot exceed FIFO_SIZE (8).
> 
> Note that in case of a NAK during transaction, the NA/ND status bits
> will be set before STOP command is generated, triggering an interrupt
> while the controller is still busy. Current solution for this problem is
> to actively wait for this command to stop before leaving xfer callback.

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
>  drivers/i2c/busses/i2c-axxia.c | 100 ++++++++++++++++++++++++++++++---
>  1 file changed, 93 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
> index 35258321e81b..64779819a948 100644
> --- a/drivers/i2c/busses/i2c-axxia.c
> +++ b/drivers/i2c/busses/i2c-axxia.c
> @@ -12,6 +12,7 @@
>   */
>  #include <linux/clk.h>
>  #include <linux/clkdev.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> @@ -25,6 +26,7 @@
>  #define I2C_XFER_TIMEOUT    (msecs_to_jiffies(250))
>  #define I2C_STOP_TIMEOUT    (msecs_to_jiffies(100))
>  #define FIFO_SIZE           8
> +#define SEQ_LEN             2
>  
>  #define GLOBAL_CONTROL		0x00
>  #define   GLOBAL_MST_EN         BIT(0)
> @@ -51,6 +53,7 @@
>  #define   CMD_BUSY		(1<<3)
>  #define   CMD_MANUAL		(0x00 | CMD_BUSY)
>  #define   CMD_AUTO		(0x01 | CMD_BUSY)
> +#define   CMD_SEQUENCE		(0x02 | CMD_BUSY)
>  #define MST_RX_XFER		0x2c
>  #define MST_TX_XFER		0x30
>  #define MST_ADDR_1		0x34
> @@ -87,7 +90,9 @@
>   * axxia_i2c_dev - I2C device context
>   * @base: pointer to register struct
>   * @msg: pointer to current message
> - * @msg_xfrd: number of bytes transferred in msg
> + * @msg_r: pointer to current read message (sequence transfer)
> + * @msg_xfrd: number of bytes transferred in tx_fifo
> + * @msg_xfrd_r: number of bytes transferred in rx_fifo
>   * @msg_err: error code for completed message
>   * @msg_complete: xfer completion object
>   * @dev: device reference
> @@ -98,7 +103,9 @@
>  struct axxia_i2c_dev {
>  	void __iomem *base;
>  	struct i2c_msg *msg;
> +	struct i2c_msg *msg_r;
>  	size_t msg_xfrd;
> +	size_t msg_xfrd_r;
>  	int msg_err;
>  	struct completion msg_complete;
>  	struct device *dev;
> @@ -227,14 +234,14 @@ static int i2c_m_recv_len(const struct i2c_msg *msg)
>   */
>  static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
>  {
> -	struct i2c_msg *msg = idev->msg;
> +	struct i2c_msg *msg = idev->msg_r;
>  	size_t rx_fifo_avail = readl(idev->base + MST_RX_FIFO);
> -	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd);
> +	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd_r);
>  
>  	while (bytes_to_transfer-- > 0) {
>  		int c = readl(idev->base + MST_DATA);
>  
> -		if (idev->msg_xfrd == 0 && i2c_m_recv_len(msg)) {
> +		if (idev->msg_xfrd_r == 0 && i2c_m_recv_len(msg)) {
>  			/*
>  			 * Check length byte for SMBus block read
>  			 */
> @@ -247,7 +254,7 @@ static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
>  			msg->len = 1 + c;
>  			writel(msg->len, idev->base + MST_RX_XFER);
>  		}
> -		msg->buf[idev->msg_xfrd++] = c;
> +		msg->buf[idev->msg_xfrd_r++] = c;
>  	}
>  
>  	return 0;
> @@ -287,7 +294,7 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  	}
>  
>  	/* RX FIFO needs service? */
> -	if (i2c_m_rd(idev->msg) && (status & MST_STATUS_RFL))
> +	if (i2c_m_rd(idev->msg_r) && (status & MST_STATUS_RFL))
>  		axxia_i2c_empty_rx_fifo(idev);
>  
>  	/* TX FIFO needs service? */
> @@ -320,9 +327,12 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  	} else if (status & MST_STATUS_SNS) {
>  		/* Transfer done */
>  		i2c_int_disable(idev, ~MST_STATUS_TSS);
> -		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
> +		if (i2c_m_rd(idev->msg_r) && idev->msg_xfrd_r < idev->msg_r->len)
>  			axxia_i2c_empty_rx_fifo(idev);
>  		complete(&idev->msg_complete);
> +	} else if (status & MST_STATUS_SS) {
> +		/* Auto/Sequence transfer done */
> +		complete(&idev->msg_complete);
>  	} else if (status & MST_STATUS_TSS) {
>  		/* Transfer timeout */
>  		idev->msg_err = -ETIMEDOUT;
> @@ -363,6 +373,62 @@ static void axxia_i2c_set_addr(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  	writel(addr_2, idev->base + MST_ADDR_2);
>  }
>  
> +/* The NAK interrupt will be sent _before_ issuing STOP command
> + * so the controller might still be busy processing it. No
> + * interrupt will be sent at the end so we have to poll for it
> + */
> +static void axxia_i2c_handle_seq_nak(struct axxia_i2c_dev *idev)
> +{
> +	while (readl(idev->base + MST_COMMAND) & CMD_BUSY)
> +		udelay(100);
> +}
> +
> +static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
> +{
> +	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SS | MST_STATUS_RFL;
> +	u32 rlen = i2c_m_recv_len(&msgs[1]) ? I2C_SMBUS_BLOCK_MAX : msgs[1].len;
> +	unsigned long time_left;
> +
> +	axxia_i2c_set_addr(idev, &msgs[0]);
> +
> +	writel(msgs[0].len, idev->base + MST_TX_XFER);
> +	writel(rlen, idev->base + MST_RX_XFER);
> +
> +	idev->msg = &msgs[0];
> +	idev->msg_r = &msgs[1];
> +	idev->msg_xfrd = 0;
> +	idev->msg_xfrd_r = 0;
> +	axxia_i2c_fill_tx_fifo(idev);
> +
> +	writel(CMD_SEQUENCE, idev->base + MST_COMMAND);
> +
> +	reinit_completion(&idev->msg_complete);
> +	i2c_int_enable(idev, int_mask);
> +
> +	time_left = wait_for_completion_timeout(&idev->msg_complete,
> +						I2C_XFER_TIMEOUT);
> +
> +	i2c_int_disable(idev, int_mask);
> +
> +	axxia_i2c_empty_rx_fifo(idev);
> +
> +	if (idev->msg_err == -ENXIO)
> +		axxia_i2c_handle_seq_nak(idev);
> +	else if (readl(idev->base + MST_COMMAND) & CMD_BUSY)
> +		dev_warn(idev->dev, "busy after xfer\n");
> +
> +	if (time_left == 0) {
> +		idev->msg_err = -ETIMEDOUT;
> +		i2c_recover_bus(&idev->adapter);
> +		axxia_i2c_init(idev);
> +	}
> +
> +	if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
> +		axxia_i2c_init(idev);
> +
> +	return idev->msg_err;
> +}
> +
>  static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  {
>  	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
> @@ -371,7 +437,9 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  	unsigned int wt_value;
>  
>  	idev->msg = msg;
> +	idev->msg_r = msg;
>  	idev->msg_xfrd = 0;
> +	idev->msg_xfrd_r = 0;
>  	reinit_completion(&idev->msg_complete);
>  
>  	axxia_i2c_set_addr(idev, msg);
> @@ -452,6 +520,18 @@ static int axxia_i2c_stop(struct axxia_i2c_dev *idev)
>  	return 0;
>  }
>  
> +/* This function checks if the msgs[] array contains messages compatible with
> + * Sequence mode of operation. This mode assumes there will be exactly one
> + * write of non-zero length followed by exactly one read of non-zero length,
> + * both targeted at the same client device.
> + */
> +static bool axxia_i2c_sequence_ok(struct i2c_msg msgs[], int num)
> +{
> +	return num == SEQ_LEN && !i2c_m_rd(&msgs[0]) && i2c_m_rd(&msgs[1]) &&
> +	       msgs[0].len > 0 && msgs[0].len <= FIFO_SIZE &&
> +	       msgs[1].len > 0 && msgs[0].addr == msgs[1].addr;
> +}
> +
>  static int
>  axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  {
> @@ -460,6 +540,12 @@ axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int ret = 0;
>  
>  	idev->msg_err = 0;
> +
> +	if (axxia_i2c_sequence_ok(msgs, num)) {
> +		ret = axxia_i2c_xfer_seq(idev, msgs);
> +		return ret ? : SEQ_LEN;
> +	}
> +
>  	i2c_int_enable(idev, MST_STATUS_TSS);
>  
>  	for (i = 0; ret == 0 && i < num; ++i)
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH 3/3] i2c-axxia: support sequence command mode

[Wolfram Sang]

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

Hi,

> +static void axxia_i2c_handle_seq_nak(struct axxia_i2c_dev *idev)
> +{
> +	while (readl(idev->base + MST_COMMAND) & CMD_BUSY)
> +		udelay(100);
> +}

My code checkers rightfully complain about this:

  CHECKPATCH
CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt

  SMATCH
drivers/i2c/busses/i2c-axxia.c:382 axxia_i2c_handle_seq_nak() warn: this loop depends on readl() succeeding

(and I get those as well, but not related to your patch; still while I am here)

  GCC
drivers/i2c/busses/i2c-axxia.c:98: warning: cannot understand function prototype: 'struct axxia_i2c_dev '
drivers/i2c/busses/i2c-axxia.c:130: warning: Function parameter or member 'ns' not described in 'ns_to_clk'
drivers/i2c/busses/i2c-axxia.c:130: warning: Function parameter or member 'clk_mhz' not described in 'ns_to_clk'
drivers/i2c/busses/i2c-axxia.c:229: warning: Function parameter or member 'idev' not described in 'axxia_i2c_empty_rx_fifo'
drivers/i2c/busses/i2c-axxia.c:261: warning: Function parameter or member 'idev' not described in 'axxia_i2c_fill_tx_fifo'

Also, for $subject, please change the prefix to "i2c: axxia:"

Thanks,

   Wolfram


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

[PATCH v2] i2c: axxia: support sequence command mode

[Adamski, Krzysztof (Nokia - PL/Wroclaw)]

In order to comply with SMBus specification, the Axxia I²C module will
abort the multi message transfer if the delay between finishing sending
one message and starting another is longer than 25ms. Unfortunately it
isn't that hard to trigger this situation on a busy system. In order to
fix this problem, we should make sure hardware does whole transaction
without waiting for software to fill some data.

Fortunately, in addition to Manual mode that is currently used by the
driver to perform I²C transfers, the module supports also so called
Sequence mode. In this mode, the module automatically performs
predefined sequence of operations - it sends a slave address, transmits
specified number of bytes from the FIFO, changes transfer direction,
resends the slave address and then reads specified number of bytes to
FIFO. While very inflexible, this does fit a most common case of multi
message transfer - the one where you first write a register number you
want to read and then read it.

To use this mode effectively, a number of conditions must be met to
ensure the transaction does fit the predefined sequence. In case this is
not the case, a fallback to manual mode is used.

The initialization of this mode is very similar to Manual mode. The most
notable difference is different bit in the Master Interrupt Status
designating finishing of transaction. Also some of the errors, like TSS,
cannot happen in this mode.

While it is possible to support transactions requesting a read of any
size (RFL interrupt will be generated when FIFO size is not enough) the
TFL interrupt is not available in this mode, thus the write part of the
transaction cannot exceed FIFO_SIZE (8).

Note that in case of a NAK during transaction, the NA/ND status bits
will be set before STOP command is generated, triggering an interrupt
while the controller is still busy. Current solution for this problem is
to actively wait for this command to stop before leaving xfer callback.

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

Sorry for those problems I still don't know why my checkpatch did not
complain about it (have to verify that) and I did add smatch to my
arsenal now.

Changes in v2:
- added timeout to axxia_i2c_handle_seq_nak()
- changed udelay to usleep_range in axxia_i2c_handle_seq_nak()

 drivers/i2c/busses/i2c-axxia.c | 108 ++++++++++++++++++++++++++++++---
 1 file changed, 101 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index 35258321e81b..03f1ce75a32e 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -12,6 +12,7 @@
  */
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
@@ -25,6 +26,7 @@
 #define I2C_XFER_TIMEOUT    (msecs_to_jiffies(250))
 #define I2C_STOP_TIMEOUT    (msecs_to_jiffies(100))
 #define FIFO_SIZE           8
+#define SEQ_LEN             2
 
 #define GLOBAL_CONTROL		0x00
 #define   GLOBAL_MST_EN         BIT(0)
@@ -51,6 +53,7 @@
 #define   CMD_BUSY		(1<<3)
 #define   CMD_MANUAL		(0x00 | CMD_BUSY)
 #define   CMD_AUTO		(0x01 | CMD_BUSY)
+#define   CMD_SEQUENCE		(0x02 | CMD_BUSY)
 #define MST_RX_XFER		0x2c
 #define MST_TX_XFER		0x30
 #define MST_ADDR_1		0x34
@@ -87,7 +90,9 @@
  * axxia_i2c_dev - I2C device context
  * @base: pointer to register struct
  * @msg: pointer to current message
- * @msg_xfrd: number of bytes transferred in msg
+ * @msg_r: pointer to current read message (sequence transfer)
+ * @msg_xfrd: number of bytes transferred in tx_fifo
+ * @msg_xfrd_r: number of bytes transferred in rx_fifo
  * @msg_err: error code for completed message
  * @msg_complete: xfer completion object
  * @dev: device reference
@@ -98,7 +103,9 @@
 struct axxia_i2c_dev {
 	void __iomem *base;
 	struct i2c_msg *msg;
+	struct i2c_msg *msg_r;
 	size_t msg_xfrd;
+	size_t msg_xfrd_r;
 	int msg_err;
 	struct completion msg_complete;
 	struct device *dev;
@@ -227,14 +234,14 @@ static int i2c_m_recv_len(const struct i2c_msg *msg)
  */
 static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
 {
-	struct i2c_msg *msg = idev->msg;
+	struct i2c_msg *msg = idev->msg_r;
 	size_t rx_fifo_avail = readl(idev->base + MST_RX_FIFO);
-	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd);
+	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd_r);
 
 	while (bytes_to_transfer-- > 0) {
 		int c = readl(idev->base + MST_DATA);
 
-		if (idev->msg_xfrd == 0 && i2c_m_recv_len(msg)) {
+		if (idev->msg_xfrd_r == 0 && i2c_m_recv_len(msg)) {
 			/*
 			 * Check length byte for SMBus block read
 			 */
@@ -247,7 +254,7 @@ static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
 			msg->len = 1 + c;
 			writel(msg->len, idev->base + MST_RX_XFER);
 		}
-		msg->buf[idev->msg_xfrd++] = c;
+		msg->buf[idev->msg_xfrd_r++] = c;
 	}
 
 	return 0;
@@ -287,7 +294,7 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 	}
 
 	/* RX FIFO needs service? */
-	if (i2c_m_rd(idev->msg) && (status & MST_STATUS_RFL))
+	if (i2c_m_rd(idev->msg_r) && (status & MST_STATUS_RFL))
 		axxia_i2c_empty_rx_fifo(idev);
 
 	/* TX FIFO needs service? */
@@ -320,9 +327,12 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 	} else if (status & MST_STATUS_SNS) {
 		/* Transfer done */
 		i2c_int_disable(idev, ~MST_STATUS_TSS);
-		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
+		if (i2c_m_rd(idev->msg_r) && idev->msg_xfrd_r < idev->msg_r->len)
 			axxia_i2c_empty_rx_fifo(idev);
 		complete(&idev->msg_complete);
+	} else if (status & MST_STATUS_SS) {
+		/* Auto/Sequence transfer done */
+		complete(&idev->msg_complete);
 	} else if (status & MST_STATUS_TSS) {
 		/* Transfer timeout */
 		idev->msg_err = -ETIMEDOUT;
@@ -363,6 +373,70 @@ static void axxia_i2c_set_addr(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	writel(addr_2, idev->base + MST_ADDR_2);
 }
 
+/* The NAK interrupt will be sent _before_ issuing STOP command
+ * so the controller might still be busy processing it. No
+ * interrupt will be sent at the end so we have to poll for it
+ */
+static int axxia_i2c_handle_seq_nak(struct axxia_i2c_dev *idev)
+{
+	unsigned long timeout = jiffies + I2C_XFER_TIMEOUT;
+
+	do {
+		if ((readl(idev->base + MST_COMMAND) & CMD_BUSY) == 0)
+			return 0;
+		usleep_range(1, 100);
+	} while (time_before(jiffies, timeout));
+
+	return -ETIMEDOUT;
+}
+
+static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
+{
+	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SS | MST_STATUS_RFL;
+	u32 rlen = i2c_m_recv_len(&msgs[1]) ? I2C_SMBUS_BLOCK_MAX : msgs[1].len;
+	unsigned long time_left;
+
+	axxia_i2c_set_addr(idev, &msgs[0]);
+
+	writel(msgs[0].len, idev->base + MST_TX_XFER);
+	writel(rlen, idev->base + MST_RX_XFER);
+
+	idev->msg = &msgs[0];
+	idev->msg_r = &msgs[1];
+	idev->msg_xfrd = 0;
+	idev->msg_xfrd_r = 0;
+	axxia_i2c_fill_tx_fifo(idev);
+
+	writel(CMD_SEQUENCE, idev->base + MST_COMMAND);
+
+	reinit_completion(&idev->msg_complete);
+	i2c_int_enable(idev, int_mask);
+
+	time_left = wait_for_completion_timeout(&idev->msg_complete,
+						I2C_XFER_TIMEOUT);
+
+	i2c_int_disable(idev, int_mask);
+
+	axxia_i2c_empty_rx_fifo(idev);
+
+	if (idev->msg_err == -ENXIO) {
+		if (axxia_i2c_handle_seq_nak(idev))
+			axxia_i2c_init(idev);
+	} else if (readl(idev->base + MST_COMMAND) & CMD_BUSY)
+		dev_warn(idev->dev, "busy after xfer\n");
+
+	if (time_left == 0) {
+		idev->msg_err = -ETIMEDOUT;
+		i2c_recover_bus(&idev->adapter);
+		axxia_i2c_init(idev);
+	}
+
+	if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
+		axxia_i2c_init(idev);
+
+	return idev->msg_err;
+}
+
 static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 {
 	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
@@ -371,7 +445,9 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	unsigned int wt_value;
 
 	idev->msg = msg;
+	idev->msg_r = msg;
 	idev->msg_xfrd = 0;
+	idev->msg_xfrd_r = 0;
 	reinit_completion(&idev->msg_complete);
 
 	axxia_i2c_set_addr(idev, msg);
@@ -452,6 +528,18 @@ static int axxia_i2c_stop(struct axxia_i2c_dev *idev)
 	return 0;
 }
 
+/* This function checks if the msgs[] array contains messages compatible with
+ * Sequence mode of operation. This mode assumes there will be exactly one
+ * write of non-zero length followed by exactly one read of non-zero length,
+ * both targeted at the same client device.
+ */
+static bool axxia_i2c_sequence_ok(struct i2c_msg msgs[], int num)
+{
+	return num == SEQ_LEN && !i2c_m_rd(&msgs[0]) && i2c_m_rd(&msgs[1]) &&
+	       msgs[0].len > 0 && msgs[0].len <= FIFO_SIZE &&
+	       msgs[1].len > 0 && msgs[0].addr == msgs[1].addr;
+}
+
 static int
 axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 {
@@ -460,6 +548,12 @@ axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int ret = 0;
 
 	idev->msg_err = 0;
+
+	if (axxia_i2c_sequence_ok(msgs, num)) {
+		ret = axxia_i2c_xfer_seq(idev, msgs);
+		return ret ? : SEQ_LEN;
+	}
+
 	i2c_int_enable(idev, MST_STATUS_TSS);
 
 	for (i = 0; ret == 0 && i < num; ++i)
-- 
2.17.2

Re: [PATCH v2] i2c: axxia: support sequence command mode

[Sverdlin, Alexander (Nokia - DE/Ulm)]

Hi!

On 13/12/2018 13:09, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> In order to comply with SMBus specification, the Axxia I²C module will
> abort the multi message transfer if the delay between finishing sending
> one message and starting another is longer than 25ms. Unfortunately it
> isn't that hard to trigger this situation on a busy system. In order to
> fix this problem, we should make sure hardware does whole transaction
> without waiting for software to fill some data.
> 
> Fortunately, in addition to Manual mode that is currently used by the
> driver to perform I²C transfers, the module supports also so called
> Sequence mode. In this mode, the module automatically performs
> predefined sequence of operations - it sends a slave address, transmits
> specified number of bytes from the FIFO, changes transfer direction,
> resends the slave address and then reads specified number of bytes to
> FIFO. While very inflexible, this does fit a most common case of multi
> message transfer - the one where you first write a register number you
> want to read and then read it.
> 
> To use this mode effectively, a number of conditions must be met to
> ensure the transaction does fit the predefined sequence. In case this is
> not the case, a fallback to manual mode is used.
> 
> The initialization of this mode is very similar to Manual mode. The most
> notable difference is different bit in the Master Interrupt Status
> designating finishing of transaction. Also some of the errors, like TSS,
> cannot happen in this mode.
> 
> While it is possible to support transactions requesting a read of any
> size (RFL interrupt will be generated when FIFO size is not enough) the
> TFL interrupt is not available in this mode, thus the write part of the
> transaction cannot exceed FIFO_SIZE (8).
> 
> Note that in case of a NAK during transaction, the NA/ND status bits
> will be set before STOP command is generated, triggering an interrupt
> while the controller is still busy. Current solution for this problem is
> to actively wait for this command to stop before leaving xfer callback.

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> ---
> 
> Sorry for those problems I still don't know why my checkpatch did not
> complain about it (have to verify that) and I did add smatch to my
> arsenal now.
> 
> Changes in v2:
> - added timeout to axxia_i2c_handle_seq_nak()
> - changed udelay to usleep_range in axxia_i2c_handle_seq_nak()
> 
>  drivers/i2c/busses/i2c-axxia.c | 108 ++++++++++++++++++++++++++++++---
>  1 file changed, 101 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
> index 35258321e81b..03f1ce75a32e 100644
> --- a/drivers/i2c/busses/i2c-axxia.c
> +++ b/drivers/i2c/busses/i2c-axxia.c
> @@ -12,6 +12,7 @@
>   */
>  #include <linux/clk.h>
>  #include <linux/clkdev.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> @@ -25,6 +26,7 @@
>  #define I2C_XFER_TIMEOUT    (msecs_to_jiffies(250))
>  #define I2C_STOP_TIMEOUT    (msecs_to_jiffies(100))
>  #define FIFO_SIZE           8
> +#define SEQ_LEN             2
>  
>  #define GLOBAL_CONTROL		0x00
>  #define   GLOBAL_MST_EN         BIT(0)
> @@ -51,6 +53,7 @@
>  #define   CMD_BUSY		(1<<3)
>  #define   CMD_MANUAL		(0x00 | CMD_BUSY)
>  #define   CMD_AUTO		(0x01 | CMD_BUSY)
> +#define   CMD_SEQUENCE		(0x02 | CMD_BUSY)
>  #define MST_RX_XFER		0x2c
>  #define MST_TX_XFER		0x30
>  #define MST_ADDR_1		0x34
> @@ -87,7 +90,9 @@
>   * axxia_i2c_dev - I2C device context
>   * @base: pointer to register struct
>   * @msg: pointer to current message
> - * @msg_xfrd: number of bytes transferred in msg
> + * @msg_r: pointer to current read message (sequence transfer)
> + * @msg_xfrd: number of bytes transferred in tx_fifo
> + * @msg_xfrd_r: number of bytes transferred in rx_fifo
>   * @msg_err: error code for completed message
>   * @msg_complete: xfer completion object
>   * @dev: device reference
> @@ -98,7 +103,9 @@
>  struct axxia_i2c_dev {
>  	void __iomem *base;
>  	struct i2c_msg *msg;
> +	struct i2c_msg *msg_r;
>  	size_t msg_xfrd;
> +	size_t msg_xfrd_r;
>  	int msg_err;
>  	struct completion msg_complete;
>  	struct device *dev;
> @@ -227,14 +234,14 @@ static int i2c_m_recv_len(const struct i2c_msg *msg)
>   */
>  static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
>  {
> -	struct i2c_msg *msg = idev->msg;
> +	struct i2c_msg *msg = idev->msg_r;
>  	size_t rx_fifo_avail = readl(idev->base + MST_RX_FIFO);
> -	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd);
> +	int bytes_to_transfer = min(rx_fifo_avail, msg->len - idev->msg_xfrd_r);
>  
>  	while (bytes_to_transfer-- > 0) {
>  		int c = readl(idev->base + MST_DATA);
>  
> -		if (idev->msg_xfrd == 0 && i2c_m_recv_len(msg)) {
> +		if (idev->msg_xfrd_r == 0 && i2c_m_recv_len(msg)) {
>  			/*
>  			 * Check length byte for SMBus block read
>  			 */
> @@ -247,7 +254,7 @@ static int axxia_i2c_empty_rx_fifo(struct axxia_i2c_dev *idev)
>  			msg->len = 1 + c;
>  			writel(msg->len, idev->base + MST_RX_XFER);
>  		}
> -		msg->buf[idev->msg_xfrd++] = c;
> +		msg->buf[idev->msg_xfrd_r++] = c;
>  	}
>  
>  	return 0;
> @@ -287,7 +294,7 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  	}
>  
>  	/* RX FIFO needs service? */
> -	if (i2c_m_rd(idev->msg) && (status & MST_STATUS_RFL))
> +	if (i2c_m_rd(idev->msg_r) && (status & MST_STATUS_RFL))
>  		axxia_i2c_empty_rx_fifo(idev);
>  
>  	/* TX FIFO needs service? */
> @@ -320,9 +327,12 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
>  	} else if (status & MST_STATUS_SNS) {
>  		/* Transfer done */
>  		i2c_int_disable(idev, ~MST_STATUS_TSS);
> -		if (i2c_m_rd(idev->msg) && idev->msg_xfrd < idev->msg->len)
> +		if (i2c_m_rd(idev->msg_r) && idev->msg_xfrd_r < idev->msg_r->len)
>  			axxia_i2c_empty_rx_fifo(idev);
>  		complete(&idev->msg_complete);
> +	} else if (status & MST_STATUS_SS) {
> +		/* Auto/Sequence transfer done */
> +		complete(&idev->msg_complete);
>  	} else if (status & MST_STATUS_TSS) {
>  		/* Transfer timeout */
>  		idev->msg_err = -ETIMEDOUT;
> @@ -363,6 +373,70 @@ static void axxia_i2c_set_addr(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  	writel(addr_2, idev->base + MST_ADDR_2);
>  }
>  
> +/* The NAK interrupt will be sent _before_ issuing STOP command
> + * so the controller might still be busy processing it. No
> + * interrupt will be sent at the end so we have to poll for it
> + */
> +static int axxia_i2c_handle_seq_nak(struct axxia_i2c_dev *idev)
> +{
> +	unsigned long timeout = jiffies + I2C_XFER_TIMEOUT;
> +
> +	do {
> +		if ((readl(idev->base + MST_COMMAND) & CMD_BUSY) == 0)
> +			return 0;
> +		usleep_range(1, 100);
> +	} while (time_before(jiffies, timeout));
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
> +{
> +	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SS | MST_STATUS_RFL;
> +	u32 rlen = i2c_m_recv_len(&msgs[1]) ? I2C_SMBUS_BLOCK_MAX : msgs[1].len;
> +	unsigned long time_left;
> +
> +	axxia_i2c_set_addr(idev, &msgs[0]);
> +
> +	writel(msgs[0].len, idev->base + MST_TX_XFER);
> +	writel(rlen, idev->base + MST_RX_XFER);
> +
> +	idev->msg = &msgs[0];
> +	idev->msg_r = &msgs[1];
> +	idev->msg_xfrd = 0;
> +	idev->msg_xfrd_r = 0;
> +	axxia_i2c_fill_tx_fifo(idev);
> +
> +	writel(CMD_SEQUENCE, idev->base + MST_COMMAND);
> +
> +	reinit_completion(&idev->msg_complete);
> +	i2c_int_enable(idev, int_mask);
> +
> +	time_left = wait_for_completion_timeout(&idev->msg_complete,
> +						I2C_XFER_TIMEOUT);
> +
> +	i2c_int_disable(idev, int_mask);
> +
> +	axxia_i2c_empty_rx_fifo(idev);
> +
> +	if (idev->msg_err == -ENXIO) {
> +		if (axxia_i2c_handle_seq_nak(idev))
> +			axxia_i2c_init(idev);
> +	} else if (readl(idev->base + MST_COMMAND) & CMD_BUSY)
> +		dev_warn(idev->dev, "busy after xfer\n");
> +
> +	if (time_left == 0) {
> +		idev->msg_err = -ETIMEDOUT;
> +		i2c_recover_bus(&idev->adapter);
> +		axxia_i2c_init(idev);
> +	}
> +
> +	if (unlikely(idev->msg_err) && idev->msg_err != -ENXIO)
> +		axxia_i2c_init(idev);
> +
> +	return idev->msg_err;
> +}
> +
>  static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  {
>  	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
> @@ -371,7 +445,9 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
>  	unsigned int wt_value;
>  
>  	idev->msg = msg;
> +	idev->msg_r = msg;
>  	idev->msg_xfrd = 0;
> +	idev->msg_xfrd_r = 0;
>  	reinit_completion(&idev->msg_complete);
>  
>  	axxia_i2c_set_addr(idev, msg);
> @@ -452,6 +528,18 @@ static int axxia_i2c_stop(struct axxia_i2c_dev *idev)
>  	return 0;
>  }
>  
> +/* This function checks if the msgs[] array contains messages compatible with
> + * Sequence mode of operation. This mode assumes there will be exactly one
> + * write of non-zero length followed by exactly one read of non-zero length,
> + * both targeted at the same client device.
> + */
> +static bool axxia_i2c_sequence_ok(struct i2c_msg msgs[], int num)
> +{
> +	return num == SEQ_LEN && !i2c_m_rd(&msgs[0]) && i2c_m_rd(&msgs[1]) &&
> +	       msgs[0].len > 0 && msgs[0].len <= FIFO_SIZE &&
> +	       msgs[1].len > 0 && msgs[0].addr == msgs[1].addr;
> +}
> +
>  static int
>  axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  {
> @@ -460,6 +548,12 @@ axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int ret = 0;
>  
>  	idev->msg_err = 0;
> +
> +	if (axxia_i2c_sequence_ok(msgs, num)) {
> +		ret = axxia_i2c_xfer_seq(idev, msgs);
> +		return ret ? : SEQ_LEN;
> +	}
> +
>  	i2c_int_enable(idev, MST_STATUS_TSS);
>  
>  	for (i = 0; ret == 0 && i < num; ++i)
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v2] i2c: axxia: support sequence command mode

[Wolfram Sang]

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

On Thu, Dec 13, 2018 at 12:09:38PM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> In order to comply with SMBus specification, the Axxia I²C module will
> abort the multi message transfer if the delay between finishing sending
> one message and starting another is longer than 25ms. Unfortunately it
> isn't that hard to trigger this situation on a busy system. In order to
> fix this problem, we should make sure hardware does whole transaction
> without waiting for software to fill some data.
> 
> Fortunately, in addition to Manual mode that is currently used by the
> driver to perform I²C transfers, the module supports also so called
> Sequence mode. In this mode, the module automatically performs
> predefined sequence of operations - it sends a slave address, transmits
> specified number of bytes from the FIFO, changes transfer direction,
> resends the slave address and then reads specified number of bytes to
> FIFO. While very inflexible, this does fit a most common case of multi
> message transfer - the one where you first write a register number you
> want to read and then read it.
> 
> To use this mode effectively, a number of conditions must be met to
> ensure the transaction does fit the predefined sequence. In case this is
> not the case, a fallback to manual mode is used.
> 
> The initialization of this mode is very similar to Manual mode. The most
> notable difference is different bit in the Master Interrupt Status
> designating finishing of transaction. Also some of the errors, like TSS,
> cannot happen in this mode.
> 
> While it is possible to support transactions requesting a read of any
> size (RFL interrupt will be generated when FIFO size is not enough) the
> TFL interrupt is not available in this mode, thus the write part of the
> transaction cannot exceed FIFO_SIZE (8).
> 
> Note that in case of a NAK during transaction, the NA/ND status bits
> will be set before STOP command is generated, triggering an interrupt
> while the controller is still busy. Current solution for this problem is
> to actively wait for this command to stop before leaving xfer callback.
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>

Fixed this 'checkpatch --strict' warning:

CHECK: braces {} should be used on all arms of this statement
#201: FILE: drivers/i2c/busses/i2c-axxia.c:422:

... and applied to for-next, thanks!


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