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@xxxxxxxxx>
> Signed-off-by: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
> ---
>
> 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.
