Message ID | 20190328111858.GA17086@localhost.localdomain |
---|---|
State | Accepted |
Headers | show |
Series | axxia-i2c: use auto cmd for last message | expand |
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
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
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>
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!
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; }