[PATCH] axxia-i2c: use auto cmd for last message

[PATCH] axxia-i2c: use auto cmd for last message

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

Some recent commits to this driver were trying to make sure the TSS
interrupt is not generated on busy system due to 25ms timer expiring
between commands. It can still happen, however if STOP command is not
issued on time at the end of the transmission. If wait_for_completion in
axxia_i2c_xfer_msg() would not return after 25ms of getting an
interrupt, TSS will be generated and idev->err_msg will be set to
-ETIMEDOUT which will be returned from the axxia_i2c_xfer_msg(), even
though the transfer did actually succeed (STOP is automatically issued
when TSS triggers).

Fortunately, apart from already used manual and sequence commands, the
controller also has so called auto command. It works just like manual
mode but it but an automatic STOP is issued when either transfer length
is met or NAK is received from slave device.

This patch changes the axxia_i2c_xfer_msg() function so that auto
command is used for last message in transaction letting hardware manage
issuing STOP. TSS is disabled just after command transferring last
message finishes. Auto command, just like sequence, ends with SS
interrupt instead of SNS so handling of both had to be unified.

The axxia_i2c_stop() is no longer needed as the transfer can only end
with following conditions:
- fully successful - then last message was send by AUTO command and STOP
  was issued automatically
- NAK received - STOP is issued automatically by controller
- arbitration lost - STOP should not be issued as we don't control the
  bus
- IP interrupt received - this is sent when transfer length is set to 0
  for auto/sequence command. The check for that is done before START is
  send so no STOP is required
- TSS received between commands - STOP is issued by the controller

Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
---
 drivers/i2c/busses/i2c-axxia.c | 57 ++++++++++++----------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-axxia.c b/drivers/i2c/busses/i2c-axxia.c
index bf564391091f..1c7b41f45c83 100644
--- a/drivers/i2c/busses/i2c-axxia.c
+++ b/drivers/i2c/busses/i2c-axxia.c
@@ -99,6 +99,7 @@
  * @adapter: core i2c abstraction
  * @i2c_clk: clock reference for i2c input clock
  * @bus_clk_rate: current i2c bus clock rate
+ * @last: a flag indicating is this is last message in transfer
  */
 struct axxia_i2c_dev {
 	void __iomem *base;
@@ -112,6 +113,7 @@ struct axxia_i2c_dev {
 	struct i2c_adapter adapter;
 	struct clk *i2c_clk;
 	u32 bus_clk_rate;
+	bool last;
 };
 
 static void i2c_int_disable(struct axxia_i2c_dev *idev, u32 mask)
@@ -324,15 +326,14 @@ static irqreturn_t axxia_i2c_isr(int irq, void *_dev)
 		/* Stop completed */
 		i2c_int_disable(idev, ~MST_STATUS_TSS);
 		complete(&idev->msg_complete);
-	} else if (status & MST_STATUS_SNS) {
+	} else if (status & (MST_STATUS_SNS | MST_STATUS_SS)) {
 		/* Transfer done */
-		i2c_int_disable(idev, ~MST_STATUS_TSS);
+		int mask = idev->last ? ~0 : ~MST_STATUS_TSS;
+
+		i2c_int_disable(idev, mask);
 		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;
@@ -405,6 +406,7 @@ static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
 	idev->msg_r = &msgs[1];
 	idev->msg_xfrd = 0;
 	idev->msg_xfrd_r = 0;
+	idev->last = true;
 	axxia_i2c_fill_tx_fifo(idev);
 
 	writel(CMD_SEQUENCE, idev->base + MST_COMMAND);
@@ -415,10 +417,6 @@ static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
 	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);
@@ -438,9 +436,10 @@ static int axxia_i2c_xfer_seq(struct axxia_i2c_dev *idev, struct i2c_msg msgs[])
 	return idev->msg_err;
 }
 
-static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
+static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg,
+			      bool last)
 {
-	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SNS;
+	u32 int_mask = MST_STATUS_ERR;
 	u32 rx_xfer, tx_xfer;
 	unsigned long time_left;
 	unsigned int wt_value;
@@ -449,6 +448,7 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	idev->msg_r = msg;
 	idev->msg_xfrd = 0;
 	idev->msg_xfrd_r = 0;
+	idev->last = last;
 	reinit_completion(&idev->msg_complete);
 
 	axxia_i2c_set_addr(idev, msg);
@@ -478,8 +478,13 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	if (idev->msg_err)
 		goto out;
 
-	/* Start manual mode */
-	writel(CMD_MANUAL, idev->base + MST_COMMAND);
+	if (!last) {
+		writel(CMD_MANUAL, idev->base + MST_COMMAND);
+		int_mask |= MST_STATUS_SNS;
+	} else {
+		writel(CMD_AUTO, idev->base + MST_COMMAND);
+		int_mask |= MST_STATUS_SS;
+	}
 
 	writel(WT_EN | wt_value, idev->base + WAIT_TIMER_CONTROL);
 
@@ -507,28 +512,6 @@ static int axxia_i2c_xfer_msg(struct axxia_i2c_dev *idev, struct i2c_msg *msg)
 	return idev->msg_err;
 }
 
-static int axxia_i2c_stop(struct axxia_i2c_dev *idev)
-{
-	u32 int_mask = MST_STATUS_ERR | MST_STATUS_SCC | MST_STATUS_TSS;
-	unsigned long time_left;
-
-	reinit_completion(&idev->msg_complete);
-
-	/* Issue stop */
-	writel(0xb, idev->base + MST_COMMAND);
-	i2c_int_enable(idev, int_mask);
-	time_left = wait_for_completion_timeout(&idev->msg_complete,
-					      I2C_STOP_TIMEOUT);
-	i2c_int_disable(idev, int_mask);
-	if (time_left == 0)
-		return -ETIMEDOUT;
-
-	if (readl(idev->base + MST_COMMAND) & CMD_BUSY)
-		dev_warn(idev->dev, "busy after stop\n");
-
-	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,
@@ -558,9 +541,7 @@ axxia_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	i2c_int_enable(idev, MST_STATUS_TSS);
 
 	for (i = 0; ret == 0 && i < num; ++i)
-		ret = axxia_i2c_xfer_msg(idev, &msgs[i]);
-
-	axxia_i2c_stop(idev);
+		ret = axxia_i2c_xfer_msg(idev, &msgs[i], i == (num - 1));
 
 	return ret ? : i;
 }
-- 
2.20.1

Re: [PATCH] axxia-i2c: use auto cmd for last message

[Wolfram Sang]

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

Hi,

On Thu, Mar 28, 2019 at 11:19:45AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> Some recent commits to this driver were trying to make sure the TSS
> interrupt is not generated on busy system due to 25ms timer expiring
> between commands. It can still happen, however if STOP command is not
> issued on time at the end of the transmission. If wait_for_completion in
> axxia_i2c_xfer_msg() would not return after 25ms of getting an
> interrupt, TSS will be generated and idev->err_msg will be set to
> -ETIMEDOUT which will be returned from the axxia_i2c_xfer_msg(), even
> though the transfer did actually succeed (STOP is automatically issued
> when TSS triggers).
> 
> Fortunately, apart from already used manual and sequence commands, the
> controller also has so called auto command. It works just like manual
> mode but it but an automatic STOP is issued when either transfer length
> is met or NAK is received from slave device.
> 
> This patch changes the axxia_i2c_xfer_msg() function so that auto
> command is used for last message in transaction letting hardware manage
> issuing STOP. TSS is disabled just after command transferring last
> message finishes. Auto command, just like sequence, ends with SS
> interrupt instead of SNS so handling of both had to be unified.
> 
> The axxia_i2c_stop() is no longer needed as the transfer can only end
> with following conditions:
> - fully successful - then last message was send by AUTO command and STOP
>   was issued automatically
> - NAK received - STOP is issued automatically by controller
> - arbitration lost - STOP should not be issued as we don't control the
>   bus
> - IP interrupt received - this is sent when transfer length is set to 0
>   for auto/sequence command. The check for that is done before START is
>   send so no STOP is required
> - TSS received between commands - STOP is issued by the controller

I am not sure. Is this a bugfix (= for-current) or more a new feature (=
for-next)?

> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

I trust you that Alexander gave the review, but it would be a tad more
'open development' if he could give it as a reply to your patch on the
mailing list.

Thanks,

   Wolfram


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

Re: [PATCH] axxia-i2c: use auto cmd for last message

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

On Wed, Apr 03, 2019 at 10:54:02PM +0200, Wolfram Sang wrote:
>Hi,
>
>On Thu, Mar 28, 2019 at 11:19:45AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
>> Some recent commits to this driver were trying to make sure the TSS
>> interrupt is not generated on busy system due to 25ms timer expiring
>> between commands. It can still happen, however if STOP command is not
>> issued on time at the end of the transmission. If wait_for_completion in
>> axxia_i2c_xfer_msg() would not return after 25ms of getting an
>> interrupt, TSS will be generated and idev->err_msg will be set to
>> -ETIMEDOUT which will be returned from the axxia_i2c_xfer_msg(), even
>> though the transfer did actually succeed (STOP is automatically issued
>> when TSS triggers).
>>
>> Fortunately, apart from already used manual and sequence commands, the
>> controller also has so called auto command. It works just like manual
>> mode but it but an automatic STOP is issued when either transfer length
>> is met or NAK is received from slave device.
>>
>> This patch changes the axxia_i2c_xfer_msg() function so that auto
>> command is used for last message in transaction letting hardware manage
>> issuing STOP. TSS is disabled just after command transferring last
>> message finishes. Auto command, just like sequence, ends with SS
>> interrupt instead of SNS so handling of both had to be unified.
>>
>> The axxia_i2c_stop() is no longer needed as the transfer can only end
>> with following conditions:
>> - fully successful - then last message was send by AUTO command and STOP
>>   was issued automatically
>> - NAK received - STOP is issued automatically by controller
>> - arbitration lost - STOP should not be issued as we don't control the
>>   bus
>> - IP interrupt received - this is sent when transfer length is set to 0
>>   for auto/sequence command. The check for that is done before START is
>>   send so no STOP is required
>> - TSS received between commands - STOP is issued by the controller
>
>I am not sure. Is this a bugfix (= for-current) or more a new feature (=
>for-next)?

Good question. I wouldn't say it is a clear bugfix and I think it would
require more creativity to justify this as a bugfix than a feature. So I
would go feature route. I might have based that in for-current, indeed
but I think it should be easily applicable on for-next as well. Or do
you want me to resubmit?

>
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
>
>I trust you that Alexander gave the review, but it would be a tad more
>'open development' if he could give it as a reply to your patch on the
>mailing list.

Fair enough. To explain myself - the patch was first reviewed and tested
inhouse before submitting it here - this is where this Reviewed-by comes
from. But lets Alexander confirm that officially.

Best regards,
Krzysztof Adamski

Re: [PATCH] axxia-i2c: use auto cmd for last message

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

Hello Wolfram,

On 03/04/2019 22:54, Wolfram Sang wrote:
>> Some recent commits to this driver were trying to make sure the TSS
>> interrupt is not generated on busy system due to 25ms timer expiring
>> between commands. It can still happen, however if STOP command is not
>> issued on time at the end of the transmission. If wait_for_completion in
>> axxia_i2c_xfer_msg() would not return after 25ms of getting an
>> interrupt, TSS will be generated and idev->err_msg will be set to
>> -ETIMEDOUT which will be returned from the axxia_i2c_xfer_msg(), even
>> though the transfer did actually succeed (STOP is automatically issued
>> when TSS triggers).
>>
>> Fortunately, apart from already used manual and sequence commands, the
>> controller also has so called auto command. It works just like manual
>> mode but it but an automatic STOP is issued when either transfer length
>> is met or NAK is received from slave device.
>>
>> This patch changes the axxia_i2c_xfer_msg() function so that auto
>> command is used for last message in transaction letting hardware manage
>> issuing STOP. TSS is disabled just after command transferring last
>> message finishes. Auto command, just like sequence, ends with SS
>> interrupt instead of SNS so handling of both had to be unified.
>>
>> The axxia_i2c_stop() is no longer needed as the transfer can only end
>> with following conditions:
>> - fully successful - then last message was send by AUTO command and STOP
>>   was issued automatically
>> - NAK received - STOP is issued automatically by controller
>> - arbitration lost - STOP should not be issued as we don't control the
>>   bus
>> - IP interrupt received - this is sent when transfer length is set to 0
>>   for auto/sequence command. The check for that is done before START is
>>   send so no STOP is required
>> - TSS received between commands - STOP is issued by the controller
> I am not sure. Is this a bugfix (= for-current) or more a new feature (=
> for-next)?
> 
>> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
>> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>
> I trust you that Alexander gave the review, but it would be a tad more
> 'open development' if he could give it as a reply to your patch on the
> mailing list.

sure, here it is:
Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH] axxia-i2c: use auto cmd for last message

[Wolfram Sang]

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

On Thu, Mar 28, 2019 at 11:19:45AM +0000, Adamski, Krzysztof (Nokia - PL/Wroclaw) wrote:
> Some recent commits to this driver were trying to make sure the TSS
> interrupt is not generated on busy system due to 25ms timer expiring
> between commands. It can still happen, however if STOP command is not
> issued on time at the end of the transmission. If wait_for_completion in
> axxia_i2c_xfer_msg() would not return after 25ms of getting an
> interrupt, TSS will be generated and idev->err_msg will be set to
> -ETIMEDOUT which will be returned from the axxia_i2c_xfer_msg(), even
> though the transfer did actually succeed (STOP is automatically issued
> when TSS triggers).
> 
> Fortunately, apart from already used manual and sequence commands, the
> controller also has so called auto command. It works just like manual
> mode but it but an automatic STOP is issued when either transfer length
> is met or NAK is received from slave device.
> 
> This patch changes the axxia_i2c_xfer_msg() function so that auto
> command is used for last message in transaction letting hardware manage
> issuing STOP. TSS is disabled just after command transferring last
> message finishes. Auto command, just like sequence, ends with SS
> interrupt instead of SNS so handling of both had to be unified.
> 
> The axxia_i2c_stop() is no longer needed as the transfer can only end
> with following conditions:
> - fully successful - then last message was send by AUTO command and STOP
>   was issued automatically
> - NAK received - STOP is issued automatically by controller
> - arbitration lost - STOP should not be issued as we don't control the
>   bus
> - IP interrupt received - this is sent when transfer length is set to 0
>   for auto/sequence command. The check for that is done before START is
>   send so no STOP is required
> - TSS received between commands - STOP is issued by the controller
> 
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@nokia.com>

Changed $subject to "i2c: axxia:" and applied to for-next, thanks!


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