2018-03-13 07:51:15

by Sricharan R

[permalink] [raw]

Subject: Re: [PATCH v2 13/13] i2c: qup: reorganization of driver code to remove polling for qup v2

Hi Abhishek,

On 3/12/2018 6:45 PM, Abhishek Sahu wrote:
> Following are the major issues in current driver code
>
> 1. The current driver simply assumes the transfer completion
> whenever its gets any non-error interrupts and then simply do the
> polling of available/free bytes in FIFO.
> 2. The block mode is not working properly since no handling in
> being done for OUT_BLOCK_WRITE_REQ and IN_BLOCK_READ_READ.
> 3. An i2c transfer can contain multiple message and QUP v2
> supports reconfiguration during run in which the mode should be same
> for all the sub transfer. Currently the mode is being programmed
> before every sub transfer which is functionally wrong. If one message
> is less than FIFO length and other message is greater than FIFO
> length, then transfers will fail.
>
> Because of above, i2c v2 transfers of size greater than 64 are failing
> with following error message
>
> i2c_qup 78b6000.i2c: timeout for fifo out full
>
> To make block mode working properly and move to use the interrupts
> instead of polling, major code reorganization is required. Following
> are the major changes done in this patch
>
> 1. Remove the polling of TX FIFO free space and RX FIFO available
> bytes and move to interrupts completely. QUP has QUP_MX_OUTPUT_DONE,
> QUP_MX_INPUT_DONE, OUT_BLOCK_WRITE_REQ and IN_BLOCK_READ_REQ
> interrupts to handle FIFO’s properly so check all these interrupts.
> 2. Determine the mode for transfer before starting by checking
> all the tx/rx data length in each message. The complete message can be
> transferred either in DMA mode or Programmed IO by FIFO/Block mode.
> in DMA mode, both tx and rx uses same mode but in PIO mode, the TX and
> RX can be in different mode.
> 3. During write, For FIFO mode, TX FIFO can be directly written
> without checking for FIFO space. For block mode, the QUP will generate
> OUT_BLOCK_WRITE_REQ interrupt whenever it has block size of available
> space.
> 4. During read, both TX and RX FIFO will be used. TX will be used
> for writing tags and RX will be used for receiving the data. In QUP,
> TX and RX can operate in separate mode so configure modes accordingly.
> 5. For read FIFO mode, wait for QUP_MX_INPUT_DONE interrupt which
> will be generated after all the bytes have been copied in RX FIFO. For
> read Block mode, QUP will generate IN_BLOCK_READ_REQ interrupts
> whenever it has block size of available data.
> 6. Split the transfer in chunk of one QUP block size(256 bytes)
> and schedule each block separately. QUP v2 supports reconfiguration
> during run in which QUP can transfer multiple blocks without issuing a
> stop events.
> 7. Port the SMBus block read support for new code changes.
>
> Signed-off-by: Abhishek Sahu <[email protected]>
> ---
>

Reviewed-by: Sricharan R <[email protected]>

Regards,
Sricharan

> * Changes from v1:
>
> 1. Removed event-based completion and changed transfer completion
> detection logic in interrupt handler
> 2. Fixed function comments as suggested in v1 review comments
> 3. Removed blk_mode_threshold from global structure
> 4. Improved determine mode logic for QUP v2 transfers
>
> drivers/i2c/busses/i2c-qup.c | 900 +++++++++++++++++++++++++------------------
> 1 file changed, 515 insertions(+), 385 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index 3bf3c34..904dfec 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -141,17 +141,40 @@
> #define DEFAULT_SRC_CLK 20000000
>
> /*
> + * Max tags length (start, stop and maximum 2 bytes address) for each QUP
> + * data transfer
> + */
> +#define QUP_MAX_TAGS_LEN 4
> +/* Max data length for each DATARD tags */
> +#define RECV_MAX_DATA_LEN 254
> +/* TAG length for DATA READ in RX FIFO */
> +#define READ_RX_TAGS_LEN 2
> +
> +/*
> * count: no of blocks
> * pos: current block number
> * tx_tag_len: tx tag length for current block
> * rx_tag_len: rx tag length for current block
> * data_len: remaining data length for current message
> + * cur_blk_len: data length for current block
> * total_tx_len: total tx length including tag bytes for current QUP transfer
> * total_rx_len: total rx length including tag bytes for current QUP transfer
> + * tx_fifo_data_pos: current byte number in TX FIFO word
> * tx_fifo_free: number of free bytes in current QUP block write.
> + * rx_fifo_data_pos: current byte number in RX FIFO word
> * fifo_available: number of available bytes in RX FIFO for current
> * QUP block read
> + * tx_fifo_data: QUP TX FIFO write works on word basis (4 bytes). New byte write
> + * to TX FIFO will be appended in this data and will be written to
> + * TX FIFO when all the 4 bytes are available.
> + * rx_fifo_data: QUP RX FIFO read works on word basis (4 bytes). This will
> + * contains the 4 bytes of RX data.
> + * cur_data: pointer to tell cur data position for current message
> + * cur_tx_tags: pointer to tell cur position in tags
> + * tx_tags_sent: all tx tag bytes have been written in FIFO word
> + * send_last_word: for tx FIFO, last word send is pending in current block
> * rx_bytes_read: if all the bytes have been read from rx FIFO.
> + * rx_tags_fetched: all the rx tag bytes have been fetched from rx fifo word
> * is_tx_blk_mode: whether tx uses block or FIFO mode in case of non BAM xfer.
> * is_rx_blk_mode: whether rx uses block or FIFO mode in case of non BAM xfer.
> * tags: contains tx tag bytes for current QUP transfer
> @@ -162,10 +185,20 @@ struct qup_i2c_block {
> int tx_tag_len;
> int rx_tag_len;
> int data_len;
> + int cur_blk_len;
> int total_tx_len;
> int total_rx_len;
> + int tx_fifo_data_pos;
> int tx_fifo_free;
> + int rx_fifo_data_pos;
> int fifo_available;
> + u32 tx_fifo_data;
> + u32 rx_fifo_data;
> + u8 *cur_data;
> + u8 *cur_tx_tags;
> + bool tx_tags_sent;
> + bool send_last_word;
> + bool rx_tags_fetched;
> bool rx_bytes_read;
> bool is_tx_blk_mode;
> bool is_rx_blk_mode;
> @@ -198,6 +231,7 @@ struct qup_i2c_dev {
> int out_blk_sz;
> int in_blk_sz;
>
> + int blk_xfer_limit;
> unsigned long one_byte_t;
> unsigned long xfer_timeout;
> struct qup_i2c_block blk;
> @@ -212,10 +246,10 @@ struct qup_i2c_dev {
>
> /* To check if this is the last msg */
> bool is_last;
> - bool is_qup_v1;
> + bool is_smbus_read;
>
> /* To configure when bus is in run state */
> - int config_run;
> + u32 config_run;
>
> /* dma parameters */
> bool is_dma;
> @@ -223,6 +257,8 @@ struct qup_i2c_dev {
> bool use_dma;
> unsigned int max_xfer_sg_len;
> unsigned int tag_buf_pos;
> + /* The threshold length above which block mode will be used */
> + unsigned int blk_mode_threshold;
> struct dma_pool *dpool;
> struct qup_i2c_tag start_tag;
> struct qup_i2c_bam brx;
> @@ -287,9 +323,6 @@ static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
> goto done;
> }
>
> - if (!qup->is_qup_v1)
> - goto done;
> -
> if (opflags & QUP_OUT_SVC_FLAG) {
> writel(QUP_OUT_SVC_FLAG, qup->base + QUP_OPERATIONAL);
>
> @@ -416,108 +449,6 @@ static int qup_i2c_bus_active(struct qup_i2c_dev *qup, int len)
> return ret;
> }
>
> -/**
> - * qup_i2c_wait_ready - wait for a give number of bytes in tx/rx path
> - * @qup: The qup_i2c_dev device
> - * @op: The bit/event to wait on
> - * @val: value of the bit to wait on, 0 or 1
> - * @len: The length the bytes to be transferred
> - */
> -static int qup_i2c_wait_ready(struct qup_i2c_dev *qup, int op, bool val,
> - int len)
> -{
> - unsigned long timeout;
> - u32 opflags;
> - u32 status;
> - u32 shift = __ffs(op);
> - int ret = 0;
> -
> - len *= qup->one_byte_t;
> - /* timeout after a wait of twice the max time */
> - timeout = jiffies + len * 4;
> -
> - for (;;) {
> - opflags = readl(qup->base + QUP_OPERATIONAL);
> - status = readl(qup->base + QUP_I2C_STATUS);
> -
> - if (((opflags & op) >> shift) == val) {
> - if ((op == QUP_OUT_NOT_EMPTY) && qup->is_last) {
> - if (!(status & I2C_STATUS_BUS_ACTIVE)) {
> - ret = 0;
> - goto done;
> - }
> - } else {
> - ret = 0;
> - goto done;
> - }
> - }
> -
> - if (time_after(jiffies, timeout)) {
> - ret = -ETIMEDOUT;
> - goto done;
> - }
> - usleep_range(len, len * 2);
> - }
> -
> -done:
> - if (qup->bus_err || qup->qup_err)
> - ret = (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
> -
> - return ret;
> -}
> -
> -static void qup_i2c_set_write_mode_v2(struct qup_i2c_dev *qup,
> - struct i2c_msg *msg)
> -{
> - /* Number of entries to shift out, including the tags */
> - int total = msg->len + qup->blk.tx_tag_len;
> -
> - total |= qup->config_run;
> -
> - if (total < qup->out_fifo_sz) {
> - /* FIFO mode */
> - writel(QUP_REPACK_EN, qup->base + QUP_IO_MODE);
> - writel(total, qup->base + QUP_MX_WRITE_CNT);
> - } else {
> - /* BLOCK mode (transfer data on chunks) */
> - writel(QUP_OUTPUT_BLK_MODE | QUP_REPACK_EN,
> - qup->base + QUP_IO_MODE);
> - writel(total, qup->base + QUP_MX_OUTPUT_CNT);
> - }
> -}
> -
> -static int check_for_fifo_space(struct qup_i2c_dev *qup)
> -{
> - int ret;
> -
> - ret = qup_i2c_change_state(qup, QUP_PAUSE_STATE);
> - if (ret)
> - goto out;
> -
> - ret = qup_i2c_wait_ready(qup, QUP_OUT_FULL,
> - RESET_BIT, 4 * ONE_BYTE);
> - if (ret) {
> - /* Fifo is full. Drain out the fifo */
> - ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
> - if (ret)
> - goto out;
> -
> - ret = qup_i2c_wait_ready(qup, QUP_OUT_NOT_EMPTY,
> - RESET_BIT, 256 * ONE_BYTE);
> - if (ret) {
> - dev_err(qup->dev, "timeout for fifo out full");
> - goto out;
> - }
> -
> - ret = qup_i2c_change_state(qup, QUP_PAUSE_STATE);
> - if (ret)
> - goto out;
> - }
> -
> -out:
> - return ret;
> -}
> -
> static void qup_i2c_write_tx_fifo_v1(struct qup_i2c_dev *qup)
> {
> struct qup_i2c_block *blk = &qup->blk;
> @@ -560,60 +491,17 @@ static void qup_i2c_write_tx_fifo_v1(struct qup_i2c_dev *qup)
> static void qup_i2c_set_blk_data(struct qup_i2c_dev *qup,
> struct i2c_msg *msg)
> {
> - memset(&qup->blk, 0, sizeof(qup->blk));
> -
> + qup->blk.pos = 0;
> qup->blk.data_len = msg->len;
> - qup->blk.count = (msg->len + QUP_READ_LIMIT - 1) / QUP_READ_LIMIT;
> -
> - /* 4 bytes for first block and 2 writes for rest */
> - qup->blk.tx_tag_len = 4 + (qup->blk.count - 1) * 2;
> -
> - /* There are 2 tag bytes that are read in to fifo for every block */
> - if (msg->flags & I2C_M_RD)
> - qup->blk.rx_tag_len = qup->blk.count * 2;
> -}
> -
> -static int qup_i2c_send_data(struct qup_i2c_dev *qup, int tlen, u8 *tbuf,
> - int dlen, u8 *dbuf)
> -{
> - u32 val = 0, idx = 0, pos = 0, i = 0, t;
> - int len = tlen + dlen;
> - u8 *buf = tbuf;
> - int ret = 0;
> -
> - while (len > 0) {
> - ret = check_for_fifo_space(qup);
> - if (ret)
> - return ret;
> -
> - t = (len >= 4) ? 4 : len;
> -
> - while (idx < t) {
> - if (!i && (pos >= tlen)) {
> - buf = dbuf;
> - pos = 0;
> - i = 1;
> - }
> - val |= buf[pos++] << (idx++ * 8);
> - }
> -
> - writel(val, qup->base + QUP_OUT_FIFO_BASE);
> - idx = 0;
> - val = 0;
> - len -= 4;
> - }
> -
> - ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
> -
> - return ret;
> + qup->blk.count = DIV_ROUND_UP(msg->len, qup->blk_xfer_limit);
> }
>
> static int qup_i2c_get_data_len(struct qup_i2c_dev *qup)
> {
> int data_len;
>
> - if (qup->blk.data_len > QUP_READ_LIMIT)
> - data_len = QUP_READ_LIMIT;
> + if (qup->blk.data_len > qup->blk_xfer_limit)
> + data_len = qup->blk_xfer_limit;
> else
> data_len = qup->blk.data_len;
>
> @@ -630,9 +518,9 @@ static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev *qup,
> {
> int len = 0;
>
> - if (msg->len > 1) {
> + if (qup->is_smbus_read) {
> tags[len++] = QUP_TAG_V2_DATARD_STOP;
> - tags[len++] = qup_i2c_get_data_len(qup) - 1;
> + tags[len++] = qup_i2c_get_data_len(qup);
> } else {
> tags[len++] = QUP_TAG_V2_START;
> tags[len++] = addr & 0xff;
> @@ -694,24 +582,6 @@ static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
> return len;
> }
>
> -static int qup_i2c_issue_xfer_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg)
> -{
> - int data_len = 0, tag_len, index;
> - int ret;
> -
> - tag_len = qup_i2c_set_tags(qup->blk.tags, qup, msg);
> - index = msg->len - qup->blk.data_len;
> -
> - /* only tags are written for read */
> - if (!(msg->flags & I2C_M_RD))
> - data_len = qup_i2c_get_data_len(qup);
> -
> - ret = qup_i2c_send_data(qup, tag_len, qup->blk.tags,
> - data_len, &msg->buf[index]);
> - qup->blk.data_len -= data_len;
> -
> - return ret;
> -}
>
> static void qup_i2c_bam_cb(void *data)
> {
> @@ -778,6 +648,7 @@ static int qup_i2c_bam_make_desc(struct qup_i2c_dev *qup, struct i2c_msg *msg)
> u32 i = 0, tlen, tx_len = 0;
> u8 *tags;
>
> + qup->blk_xfer_limit = QUP_READ_LIMIT;
> qup_i2c_set_blk_data(qup, msg);
>
> blocks = qup->blk.count;
> @@ -1026,7 +897,7 @@ static int qup_i2c_wait_for_complete(struct qup_i2c_dev *qup,
> unsigned long left;
> int ret = 0;
>
> - left = wait_for_completion_timeout(&qup->xfer, HZ);
> + left = wait_for_completion_timeout(&qup->xfer, qup->xfer_timeout);
> if (!left) {
> writel(1, qup->base + QUP_SW_RESET);
> ret = -ETIMEDOUT;
> @@ -1038,65 +909,6 @@ static int qup_i2c_wait_for_complete(struct qup_i2c_dev *qup,
> return ret;
> }
>
> -static int qup_i2c_write_one_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg)
> -{
> - int ret = 0;
> -
> - qup->msg = msg;
> - qup->pos = 0;
> - enable_irq(qup->irq);
> - qup_i2c_set_blk_data(qup, msg);
> - qup_i2c_set_write_mode_v2(qup, msg);
> -
> - ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
> - if (ret)
> - goto err;
> -
> - writel(qup->clk_ctl, qup->base + QUP_I2C_CLK_CTL);
> -
> - do {
> - ret = qup_i2c_issue_xfer_v2(qup, msg);
> - if (ret)
> - goto err;
> -
> - ret = qup_i2c_wait_for_complete(qup, msg);
> - if (ret)
> - goto err;
> -
> - qup->blk.pos++;
> - } while (qup->blk.pos < qup->blk.count);
> -
> - ret = qup_i2c_wait_ready(qup, QUP_OUT_NOT_EMPTY, RESET_BIT, ONE_BYTE);
> -
> -err:
> - disable_irq(qup->irq);
> - qup->msg = NULL;
> -
> - return ret;
> -}
> -
> -static void qup_i2c_set_read_mode_v2(struct qup_i2c_dev *qup, int len)
> -{
> - int tx_len = qup->blk.tx_tag_len;
> -
> - len += qup->blk.rx_tag_len;
> - len |= qup->config_run;
> - tx_len |= qup->config_run;
> -
> - if (len < qup->in_fifo_sz) {
> - /* FIFO mode */
> - writel(QUP_REPACK_EN, qup->base + QUP_IO_MODE);
> - writel(tx_len, qup->base + QUP_MX_WRITE_CNT);
> - writel(len, qup->base + QUP_MX_READ_CNT);
> - } else {
> - /* BLOCK mode (transfer data on chunks) */
> - writel(QUP_INPUT_BLK_MODE | QUP_REPACK_EN,
> - qup->base + QUP_IO_MODE);
> - writel(tx_len, qup->base + QUP_MX_OUTPUT_CNT);
> - writel(len, qup->base + QUP_MX_INPUT_CNT);
> - }
> -}
> -
> static void qup_i2c_read_rx_fifo_v1(struct qup_i2c_dev *qup)
> {
> struct qup_i2c_block *blk = &qup->blk;
> @@ -1120,104 +932,6 @@ static void qup_i2c_read_rx_fifo_v1(struct qup_i2c_dev *qup)
> blk->rx_bytes_read = true;
> }
>
> -static int qup_i2c_read_fifo_v2(struct qup_i2c_dev *qup,
> - struct i2c_msg *msg)
> -{
> - u32 val;
> - int idx, pos = 0, ret = 0, total, msg_offset = 0;
> -
> - /*
> - * If the message length is already read in
> - * the first byte of the buffer, account for
> - * that by setting the offset
> - */
> - if (qup_i2c_check_msg_len(msg) && (msg->len > 1))
> - msg_offset = 1;
> - total = qup_i2c_get_data_len(qup);
> - total -= msg_offset;
> -
> - /* 2 extra bytes for read tags */
> - while (pos < (total + 2)) {
> - /* Check that FIFO have data */
> - ret = qup_i2c_wait_ready(qup, QUP_IN_NOT_EMPTY,
> - SET_BIT, 4 * ONE_BYTE);
> - if (ret) {
> - dev_err(qup->dev, "timeout for fifo not empty");
> - return ret;
> - }
> - val = readl(qup->base + QUP_IN_FIFO_BASE);
> -
> - for (idx = 0; idx < 4; idx++, val >>= 8, pos++) {
> - /* first 2 bytes are tag bytes */
> - if (pos < 2)
> - continue;
> -
> - if (pos >= (total + 2))
> - goto out;
> - msg->buf[qup->pos + msg_offset] = val & 0xff;
> - qup->pos++;
> - }
> - }
> -
> -out:
> - qup->blk.data_len -= total;
> -
> - return ret;
> -}
> -
> -static int qup_i2c_read_one_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg)
> -{
> - int ret = 0;
> -
> - qup->msg = msg;
> - qup->pos = 0;
> - enable_irq(qup->irq);
> - qup_i2c_set_blk_data(qup, msg);
> - qup_i2c_set_read_mode_v2(qup, msg->len);
> -
> - ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
> - if (ret)
> - goto err;
> -
> - writel(qup->clk_ctl, qup->base + QUP_I2C_CLK_CTL);
> -
> - do {
> - ret = qup_i2c_issue_xfer_v2(qup, msg);
> - if (ret)
> - goto err;
> -
> - ret = qup_i2c_wait_for_complete(qup, msg);
> - if (ret)
> - goto err;
> -
> - ret = qup_i2c_read_fifo_v2(qup, msg);
> - if (ret)
> - goto err;
> -
> - qup->blk.pos++;
> -
> - /* Handle SMBus block read length */
> - if (qup_i2c_check_msg_len(msg) && (msg->len == 1)) {
> - if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) {
> - ret = -EPROTO;
> - goto err;
> - }
> - msg->len += msg->buf[0];
> - qup->pos = 0;
> - qup_i2c_set_blk_data(qup, msg);
> - /* set tag length for block read */
> - qup->blk.tx_tag_len = 2;
> - qup_i2c_set_read_mode_v2(qup, msg->buf[0]);
> - }
> - } while (qup->blk.pos < qup->blk.count);
> -
> -err:
> - disable_irq(qup->irq);
> - qup->msg = NULL;
> -
> - return ret;
> -}
> -
> static void qup_i2c_write_rx_tags_v1(struct qup_i2c_dev *qup)
> {
> struct i2c_msg *msg = qup->msg;
> @@ -1404,13 +1118,434 @@ static int qup_i2c_xfer(struct i2c_adapter *adap,
> return ret;
> }
>
> +/*
> + * Configure registers related with reconfiguration during run and call it
> + * before each i2c sub transfer.
> + */
> +static void qup_i2c_conf_count_v2(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> + u32 qup_config = I2C_MINI_CORE | I2C_N_VAL_V2;
> +
> + if (blk->is_tx_blk_mode)
> + writel(qup->config_run | blk->total_tx_len,
> + qup->base + QUP_MX_OUTPUT_CNT);
> + else
> + writel(qup->config_run | blk->total_tx_len,
> + qup->base + QUP_MX_WRITE_CNT);
> +
> + if (blk->total_rx_len) {
> + if (blk->is_rx_blk_mode)
> + writel(qup->config_run | blk->total_rx_len,
> + qup->base + QUP_MX_INPUT_CNT);
> + else
> + writel(qup->config_run | blk->total_rx_len,
> + qup->base + QUP_MX_READ_CNT);
> + } else {
> + qup_config |= QUP_NO_INPUT;
> + }
> +
> + writel(qup_config, qup->base + QUP_CONFIG);
> +}
> +
> +/*
> + * Configure registers related with transfer mode (FIFO/Block)
> + * before starting of i2c transfer. It will be called only once in
> + * QUP RESET state.
> + */
> +static void qup_i2c_conf_mode_v2(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> + u32 io_mode = QUP_REPACK_EN;
> +
> + if (blk->is_tx_blk_mode) {
> + io_mode |= QUP_OUTPUT_BLK_MODE;
> + writel(0, qup->base + QUP_MX_WRITE_CNT);
> + } else {
> + writel(0, qup->base + QUP_MX_OUTPUT_CNT);
> + }
> +
> + if (blk->is_rx_blk_mode) {
> + io_mode |= QUP_INPUT_BLK_MODE;
> + writel(0, qup->base + QUP_MX_READ_CNT);
> + } else {
> + writel(0, qup->base + QUP_MX_INPUT_CNT);
> + }
> +
> + writel(io_mode, qup->base + QUP_IO_MODE);
> +}
> +
> +/* Clear required variables before starting of any QUP v2 sub transfer. */
> +static void qup_i2c_clear_blk_v2(struct qup_i2c_block *blk)
> +{
> + blk->send_last_word = false;
> + blk->tx_tags_sent = false;
> + blk->tx_fifo_data = 0;
> + blk->tx_fifo_data_pos = 0;
> + blk->tx_fifo_free = 0;
> +
> + blk->rx_tags_fetched = false;
> + blk->rx_bytes_read = false;
> + blk->rx_fifo_data = 0;
> + blk->rx_fifo_data_pos = 0;
> + blk->fifo_available = 0;
> +}
> +
> +/* Receive data from RX FIFO for read message in QUP v2 i2c transfer. */
> +static void qup_i2c_recv_data(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> + int j;
> +
> + for (j = blk->rx_fifo_data_pos;
> + blk->cur_blk_len && blk->fifo_available;
> + blk->cur_blk_len--, blk->fifo_available--) {
> + if (j == 0)
> + blk->rx_fifo_data = readl(qup->base + QUP_IN_FIFO_BASE);
> +
> + *(blk->cur_data++) = blk->rx_fifo_data;
> + blk->rx_fifo_data >>= 8;
> +
> + if (j == 3)
> + j = 0;
> + else
> + j++;
> + }
> +
> + blk->rx_fifo_data_pos = j;
> +}
> +
> +/* Receive tags for read message in QUP v2 i2c transfer. */
> +static void qup_i2c_recv_tags(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> +
> + blk->rx_fifo_data = readl(qup->base + QUP_IN_FIFO_BASE);
> + blk->rx_fifo_data >>= blk->rx_tag_len * 8;
> + blk->rx_fifo_data_pos = blk->rx_tag_len;
> + blk->fifo_available -= blk->rx_tag_len;
> +}
> +
> +/*
> + * Read the data and tags from RX FIFO. Since in read case, the tags will be
> + * preceded by received data bytes so
> + * 1. Check if rx_tags_fetched is false i.e. the start of QUP block so receive
> + * all tag bytes and discard that.
> + * 2. Read the data from RX FIFO. When all the data bytes have been read then
> + * set rx_bytes_read to true.
> + */
> +static void qup_i2c_read_rx_fifo_v2(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> +
> + if (!blk->rx_tags_fetched) {
> + qup_i2c_recv_tags(qup);
> + blk->rx_tags_fetched = true;
> + }
> +
> + qup_i2c_recv_data(qup);
> + if (!blk->cur_blk_len)
> + blk->rx_bytes_read = true;
> +}
> +
> +/*
> + * Write bytes in TX FIFO for write message in QUP v2 i2c transfer. QUP TX FIFO
> + * write works on word basis (4 bytes). Append new data byte write for TX FIFO
> + * in tx_fifo_data and write to TX FIFO when all the 4 bytes are present.
> + */
> +static void
> +qup_i2c_write_blk_data(struct qup_i2c_dev *qup, u8 **data, unsigned int *len)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> + unsigned int j;
> +
> + for (j = blk->tx_fifo_data_pos; *len && blk->tx_fifo_free;
> + (*len)--, blk->tx_fifo_free--) {
> + blk->tx_fifo_data |= *(*data)++ << (j * 8);
> + if (j == 3) {
> + writel(blk->tx_fifo_data,
> + qup->base + QUP_OUT_FIFO_BASE);
> + blk->tx_fifo_data = 0x0;
> + j = 0;
> + } else {
> + j++;
> + }
> + }
> +
> + blk->tx_fifo_data_pos = j;
> +}
> +
> +/* Transfer tags for read message in QUP v2 i2c transfer. */
> +static void qup_i2c_write_rx_tags_v2(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> +
> + qup_i2c_write_blk_data(qup, &blk->cur_tx_tags, &blk->tx_tag_len);
> + if (blk->tx_fifo_data_pos)
> + writel(blk->tx_fifo_data, qup->base + QUP_OUT_FIFO_BASE);
> +}
> +
> +/*
> + * Write the data and tags in TX FIFO. Since in write case, both tags and data
> + * need to be written and QUP write tags can have maximum 256 data length, so
> + *
> + * 1. Check if tx_tags_sent is false i.e. the start of QUP block so write the
> + * tags to TX FIFO and set tx_tags_sent to true.
> + * 2. Check if send_last_word is true. It will be set when last few data bytes
> + * (less than 4 bytes) are reamining to be written in FIFO because of no FIFO
> + * space. All this data bytes are available in tx_fifo_data so write this
> + * in FIFO.
> + * 3. Write the data to TX FIFO and check for cur_blk_len. If it is non zero
> + * then more data is pending otherwise following 3 cases can be possible
> + * a. if tx_fifo_data_pos is zero i.e. all the data bytes in this block
> + * have been written in TX FIFO so nothing else is required.
> + * b. tx_fifo_free is non zero i.e tx FIFO is free so copy the remaining data
> + * from tx_fifo_data to tx FIFO. Since, qup_i2c_write_blk_data do write
> + * in 4 bytes and FIFO space is in multiple of 4 bytes so tx_fifo_free
> + * will be always greater than or equal to 4 bytes.
> + * c. tx_fifo_free is zero. In this case, last few bytes (less than 4
> + * bytes) are copied to tx_fifo_data but couldn't be sent because of
> + * FIFO full so make send_last_word true.
> + */
> +static void qup_i2c_write_tx_fifo_v2(struct qup_i2c_dev *qup)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> +
> + if (!blk->tx_tags_sent) {
> + qup_i2c_write_blk_data(qup, &blk->cur_tx_tags,
> + &blk->tx_tag_len);
> + blk->tx_tags_sent = true;
> + }
> +
> + if (blk->send_last_word)
> + goto send_last_word;
> +
> + qup_i2c_write_blk_data(qup, &blk->cur_data, &blk->cur_blk_len);
> + if (!blk->cur_blk_len) {
> + if (!blk->tx_fifo_data_pos)
> + return;
> +
> + if (blk->tx_fifo_free)
> + goto send_last_word;
> +
> + blk->send_last_word = true;
> + }
> +
> + return;
> +
> +send_last_word:
> + writel(blk->tx_fifo_data, qup->base + QUP_OUT_FIFO_BASE);
> +}
> +
> +/*
> + * Main transfer function which read or write i2c data.
> + * The QUP v2 supports reconfiguration during run in which multiple i2c sub
> + * transfers can be scheduled.
> + */
> +static int
> +qup_i2c_conf_xfer_v2(struct qup_i2c_dev *qup, bool is_rx, bool is_first,
> + bool change_pause_state)
> +{
> + struct qup_i2c_block *blk = &qup->blk;
> + struct i2c_msg *msg = qup->msg;
> + int ret;
> +
> + /*
> + * Check if its SMBus Block read for which the top level read will be
> + * done into 2 QUP reads. One with message length 1 while other one is
> + * with actual length.
> + */
> + if (qup_i2c_check_msg_len(msg)) {
> + if (qup->is_smbus_read) {
> + /*
> + * If the message length is already read in
> + * the first byte of the buffer, account for
> + * that by setting the offset
> + */
> + blk->cur_data += 1;
> + is_first = false;
> + } else {
> + change_pause_state = false;
> + }
> + }
> +
> + qup->config_run = is_first ? 0 : QUP_I2C_MX_CONFIG_DURING_RUN;
> +
> + qup_i2c_clear_blk_v2(blk);
> + qup_i2c_conf_count_v2(qup);
> +
> + /* If it is first sub transfer, then configure i2c bus clocks */
> + if (is_first) {
> + ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
> + if (ret)
> + return ret;
> +
> + writel(qup->clk_ctl, qup->base + QUP_I2C_CLK_CTL);
> +
> + ret = qup_i2c_change_state(qup, QUP_PAUSE_STATE);
> + if (ret)
> + return ret;
> + }
> +
> + reinit_completion(&qup->xfer);
> + enable_irq(qup->irq);
> + /*
> + * In FIFO mode, tx FIFO can be written directly while in block mode the
> + * it will be written after getting OUT_BLOCK_WRITE_REQ interrupt
> + */
> + if (!blk->is_tx_blk_mode) {
> + blk->tx_fifo_free = qup->out_fifo_sz;
> +
> + if (is_rx)
> + qup_i2c_write_rx_tags_v2(qup);
> + else
> + qup_i2c_write_tx_fifo_v2(qup);
> + }
> +
> + ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
> + if (ret)
> + goto err;
> +
> + ret = qup_i2c_wait_for_complete(qup, msg);
> + if (ret)
> + goto err;
> +
> + /* Move to pause state for all the transfers, except last one */
> + if (change_pause_state) {
> + ret = qup_i2c_change_state(qup, QUP_PAUSE_STATE);
> + if (ret)
> + goto err;
> + }
> +
> +err:
> + disable_irq(qup->irq);
> + return ret;
> +}
> +
> +/*
> + * Transfer one read/write message in i2c transfer. It splits the message into
> + * multiple of blk_xfer_limit data length blocks and schedule each
> + * QUP block individually.
> + */
> +static int qup_i2c_xfer_v2_msg(struct qup_i2c_dev *qup, int msg_id, bool is_rx)
> +{
> + int ret = 0;
> + unsigned int data_len, i;
> + struct i2c_msg *msg = qup->msg;
> + struct qup_i2c_block *blk = &qup->blk;
> + u8 *msg_buf = msg->buf;
> +
> + qup->blk_xfer_limit = is_rx ? RECV_MAX_DATA_LEN : QUP_READ_LIMIT;
> + qup_i2c_set_blk_data(qup, msg);
> +
> + for (i = 0; i < blk->count; i++) {
> + data_len = qup_i2c_get_data_len(qup);
> + blk->pos = i;
> + blk->cur_tx_tags = blk->tags;
> + blk->cur_blk_len = data_len;
> + blk->tx_tag_len =
> + qup_i2c_set_tags(blk->cur_tx_tags, qup, qup->msg);
> +
> + blk->cur_data = msg_buf;
> +
> + if (is_rx) {
> + blk->total_tx_len = blk->tx_tag_len;
> + blk->rx_tag_len = 2;
> + blk->total_rx_len = blk->rx_tag_len + data_len;
> + } else {
> + blk->total_tx_len = blk->tx_tag_len + data_len;
> + blk->total_rx_len = 0;
> + }
> +
> + ret = qup_i2c_conf_xfer_v2(qup, is_rx, !msg_id && !i,
> + !qup->is_last || i < blk->count - 1);
> + if (ret)
> + return ret;
> +
> + /* Handle SMBus block read length */
> + if (qup_i2c_check_msg_len(msg) && msg->len == 1 &&
> + !qup->is_smbus_read) {
> + if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX)
> + return -EPROTO;
> +
> + msg->len = msg->buf[0];
> + qup->is_smbus_read = true;
> + ret = qup_i2c_xfer_v2_msg(qup, msg_id, true);
> + qup->is_smbus_read = false;
> + if (ret)
> + return ret;
> +
> + msg->len += 1;
> + }
> +
> + msg_buf += data_len;
> + blk->data_len -= qup->blk_xfer_limit;
> + }
> +
> + return ret;
> +}
> +
> +/*
> + * QUP v2 supports 3 modes
> + * Programmed IO using FIFO mode : Less than FIFO size
> + * Programmed IO using Block mode : Greater than FIFO size
> + * DMA using BAM : Appropriate for any transaction size but the address should
> + * be DMA applicable
> + *
> + * This function determines the mode which will be used for this transfer. An
> + * i2c transfer contains multiple message. Following are the rules to determine
> + * the mode used.
> + * 1. Determine complete length, maximum tx and rx length for complete transfer.
> + * 2. If complete transfer length is greater than fifo size then use the DMA
> + * mode.
> + * 3. In FIFO or block mode, tx and rx can operate in different mode so check
> + * for maximum tx and rx length to determine mode.
> + */
> +static int
> +qup_i2c_determine_mode_v2(struct qup_i2c_dev *qup,
> + struct i2c_msg msgs[], int num)
> +{
> + int idx;
> + bool no_dma = false;
> + unsigned int max_tx_len = 0, max_rx_len = 0, total_len = 0;
> +
> + /* All i2c_msgs should be transferred using either dma or cpu */
> + for (idx = 0; idx < num; idx++) {
> + if (msgs[idx].len == 0)
> + return -EINVAL;
> +
> + if (msgs[idx].flags & I2C_M_RD)
> + max_rx_len = max_t(unsigned int, max_rx_len,
> + msgs[idx].len);
> + else
> + max_tx_len = max_t(unsigned int, max_tx_len,
> + msgs[idx].len);
> +
> + if (is_vmalloc_addr(msgs[idx].buf))
> + no_dma = true;
> +
> + total_len += msgs[idx].len;
> + }
> +
> + if (!no_dma && qup->is_dma &&
> + (total_len > qup->out_fifo_sz || total_len > qup->in_fifo_sz)) {
> + qup->use_dma = true;
> + } else {
> + qup->blk.is_tx_blk_mode = max_tx_len > qup->out_fifo_sz -
> + QUP_MAX_TAGS_LEN ? true : false;
> + qup->blk.is_rx_blk_mode = max_rx_len > qup->in_fifo_sz -
> + READ_RX_TAGS_LEN ? true : false;
> + }
> +
> + return 0;
> +}
> +
> static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
> struct i2c_msg msgs[],
> int num)
> {
> struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
> int ret, idx = 0;
> - unsigned int total_len = 0;
>
> qup->bus_err = 0;
> qup->qup_err = 0;
> @@ -1419,6 +1554,10 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
> if (ret < 0)
> goto out;
>
> + ret = qup_i2c_determine_mode_v2(qup, msgs, num);
> + if (ret)
> + goto out;
> +
> writel(1, qup->base + QUP_SW_RESET);
> ret = qup_i2c_poll_state(qup, QUP_RESET_STATE);
> if (ret)
> @@ -1428,60 +1567,35 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
> writel(I2C_MINI_CORE | I2C_N_VAL_V2, qup->base + QUP_CONFIG);
> writel(QUP_V2_TAGS_EN, qup->base + QUP_I2C_MASTER_GEN);
>
> - if ((qup->is_dma)) {
> - /* All i2c_msgs should be transferred using either dma or cpu */
> + if (qup_i2c_poll_state_i2c_master(qup)) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + if (qup->use_dma) {
> + reinit_completion(&qup->xfer);
> + ret = qup_i2c_bam_xfer(adap, &msgs[0], num);
> + qup->use_dma = false;
> + } else {
> + qup_i2c_conf_mode_v2(qup);
> +
> for (idx = 0; idx < num; idx++) {
> - if (msgs[idx].len == 0) {
> - ret = -EINVAL;
> - goto out;
> - }
> + qup->msg = &msgs[idx];
> + qup->is_last = idx == (num - 1);
>
> - if (is_vmalloc_addr(msgs[idx].buf))
> + ret = qup_i2c_xfer_v2_msg(qup, idx,
> + !!(msgs[idx].flags & I2C_M_RD));
> + if (ret)
> break;
> -
> - total_len += msgs[idx].len;
> }
> -
> - if (idx == num && (total_len > qup->out_fifo_sz ||
> - total_len > qup->in_fifo_sz))
> - qup->use_dma = true;
> + qup->msg = NULL;
> }
>
> - idx = 0;
> -
> - do {
> - if (msgs[idx].len == 0) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - if (qup_i2c_poll_state_i2c_master(qup)) {
> - ret = -EIO;
> - goto out;
> - }
> -
> - qup->is_last = (idx == (num - 1));
> - if (idx)
> - qup->config_run = QUP_I2C_MX_CONFIG_DURING_RUN;
> - else
> - qup->config_run = 0;
> -
> - reinit_completion(&qup->xfer);
> -
> - if (qup->use_dma) {
> - ret = qup_i2c_bam_xfer(adap, &msgs[idx], num);
> - qup->use_dma = false;
> - break;
> - } else {
> - if (msgs[idx].flags & I2C_M_RD)
> - ret = qup_i2c_read_one_v2(qup, &msgs[idx]);
> - else
> - ret = qup_i2c_write_one_v2(qup, &msgs[idx]);
> - }
> - } while ((idx++ < (num - 1)) && !ret);
> + if (!ret)
> + ret = qup_i2c_bus_active(qup, ONE_BYTE);
>
> if (!ret)
> - ret = qup_i2c_change_state(qup, QUP_RESET_STATE);
> + qup_i2c_change_state(qup, QUP_RESET_STATE);
>
> if (ret == 0)
> ret = num;
> @@ -1545,6 +1659,7 @@ static int qup_i2c_probe(struct platform_device *pdev)
> u32 src_clk_freq = DEFAULT_SRC_CLK;
> u32 clk_freq = DEFAULT_CLK_FREQ;
> int blocks;
> + bool is_qup_v1;
>
> qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
> if (!qup)
> @@ -1563,12 +1678,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
> if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-v1.1.1")) {
> qup->adap.algo = &qup_i2c_algo;
> qup->adap.quirks = &qup_i2c_quirks;
> - qup->is_qup_v1 = true;
> - qup->write_tx_fifo = qup_i2c_write_tx_fifo_v1;
> - qup->read_rx_fifo = qup_i2c_read_rx_fifo_v1;
> - qup->write_rx_tags = qup_i2c_write_rx_tags_v1;
> + is_qup_v1 = true;
> } else {
> qup->adap.algo = &qup_i2c_algo_v2;
> + is_qup_v1 = false;
> ret = qup_i2c_req_dma(qup);
>
> if (ret == -EPROBE_DEFER)
> @@ -1694,14 +1807,31 @@ static int qup_i2c_probe(struct platform_device *pdev)
> ret = -EIO;
> goto fail;
> }
> - qup->out_blk_sz = blk_sizes[size] / 2;
> + qup->out_blk_sz = blk_sizes[size];
>
> size = QUP_INPUT_BLOCK_SIZE(io_mode);
> if (size >= ARRAY_SIZE(blk_sizes)) {
> ret = -EIO;
> goto fail;
> }
> - qup->in_blk_sz = blk_sizes[size] / 2;
> + qup->in_blk_sz = blk_sizes[size];
> +
> + if (is_qup_v1) {
> + /*
> + * in QUP v1, QUP_CONFIG uses N as 15 i.e 16 bits constitutes a
> + * single transfer but the block size is in bytes so divide the
> + * in_blk_sz and out_blk_sz by 2
> + */
> + qup->in_blk_sz /= 2;
> + qup->out_blk_sz /= 2;
> + qup->write_tx_fifo = qup_i2c_write_tx_fifo_v1;
> + qup->read_rx_fifo = qup_i2c_read_rx_fifo_v1;
> + qup->write_rx_tags = qup_i2c_write_rx_tags_v1;
> + } else {
> + qup->write_tx_fifo = qup_i2c_write_tx_fifo_v2;
> + qup->read_rx_fifo = qup_i2c_read_rx_fifo_v2;
> + qup->write_rx_tags = qup_i2c_write_rx_tags_v2;
> + }
>
> size = QUP_OUTPUT_FIFO_SIZE(io_mode);
> qup->out_fifo_sz = qup->out_blk_sz * (2 << size);
>

--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

2018-03-13 21:11:25

by Christ, Austin

[permalink] [raw]

Subject: Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working

I've tested this v2 series on Centriq 2400. Looks good to me.

Reviewed-by: Austin Christ <[email protected]>

On 3/12/2018 7:14 AM, Abhishek Sahu wrote:
> * v2:
>
> 1. Address review comments in v1
> 2. Changed the license to SPDX
> 3. Changed commit messages for some of the patch having more detail
> 4. Removed event-based completion and changed transfer completion
> detection logic in interrupt handler
> 5. Removed dma_threshold and blk_mode_threshold from global structure
> 6. Improved determine mode logic for QUP v2 transfers
> 7. Fixed function comments
> 8. Fixed auto build test WARNING ‘idx' may be used uninitialized
> in this function
> 9. Renamed tx/rx_buf to tx/rx_cnt
>
> * v1:
>
> The current driver is failing in following test case
> 1. Handling of failure cases is not working in long run for BAM
> mode. It generates error message “bam-dma-engine 7884000.dma: Cannot
> free busy channel” sometimes.
> 2. Following I2C transfers are failing
> a. Single transfer with multiple read messages
> b. Single transfer with multiple read/write message with maximum
> allowed length per message (65K) in BAM mode
> c. Single transfer with write greater than 32 bytes in QUP v1 and
> write greater than 64 bytes in QUP v2 for non-DMA mode.
> 3. No handling is present for Block/FIFO interrupts. Any non-error
> interrupts are being treated as the transfer completion and then
> polling is being done for available/free bytes in FIFO.
>
> To fix all these issues, major code changes are required. This patch
> series fixes all the above issues and makes the driver interrupt based
> instead of polling based. After these changes, all the mentioned test
> cases are working properly.
>
> The code changes have been tested for QUP v1 (IPQ8064) and QUP
> v2 (IPQ8074) with sample application written over i2c-dev.
>
> Abhishek Sahu (13):
> i2c: qup: fix copyrights and update to SPDX identifier
> i2c: qup: fixed releasing dma without flush operation completion
> i2c: qup: minor code reorganization for use_dma
> i2c: qup: remove redundant variables for BAM SG count
> i2c: qup: schedule EOT and FLUSH tags at the end of transfer
> i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags
> i2c: qup: proper error handling for i2c error in BAM mode
> i2c: qup: use the complete transfer length to choose DMA mode
> i2c: qup: change completion timeout according to transfer length
> i2c: qup: fix buffer overflow for multiple msg of maximum xfer len
> i2c: qup: send NACK for last read sub transfers
> i2c: qup: reorganization of driver code to remove polling for qup v1
> i2c: qup: reorganization of driver code to remove polling for qup v2
>
> drivers/i2c/busses/i2c-qup.c | 1507 ++++++++++++++++++++++++------------------
> 1 file changed, 880 insertions(+), 627 deletions(-)
>

--
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

2018-03-13 21:19:10

On Mon, Mar 12, 2018 at 06:44:50PM +0530, Abhishek Sahu wrote:
> The file has been updated from 2016 to 2018 so fixed the
> copyright years.
>
> Signed-off-by: Abhishek Sahu <[email protected]>
> ---
> drivers/i2c/busses/i2c-qup.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index 08f8e01..ac5edfa 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -1,17 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2009-2013, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2009-2013, 2016-2018, The Linux Foundation. All rights reserved.

Can we do this? I know that CodeAurora is hosted by LF, but can you
extend their copyright when working for Qualcomm?

> * Copyright (c) 2014, Sony Mobile Communications AB.
> *
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 and
> - * only version 2 as published by the Free Software Foundation.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> */
>
> #include <linux/acpi.h>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>

Attachments:

(No filename) (1.57 kB)
signature.asc (849.00 B)
Download all attachments

2018-03-17 20:41:23

by Wolfram Sang

[permalink] [raw]

Subject: Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working

I trust the reviews of Andy and Sricharan for this series. From what I
looked at, the patches look good to me. Only one question left about
copyrights (raised seperately), but we are good to go I think.

Thanks for all the review!

Attachments:

(No filename) (240.00 B)
signature.asc (849.00 B)
Download all attachments

2018-03-17 21:03:13

> Qualcomm does not uses its own copyright for open source and uses
> LF copyright only.

Didn't know that. Strange, but ok.

> Following is the downstream version available in CAF
> for the same i2c-qup.c file in which the LF copyright has
> already extended.
>
> https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-msm/tree/drivers/i2c/busses/i2c-qup.c?h=eggplant

Thanks, this is good enough for me.

Attachments:

(No filename) (430.00 B)
signature.asc (849.00 B)
Download all attachments

2018-03-24 12:23:18

by Wolfram Sang

[permalink] [raw]

Subject: Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working

On Mon, Mar 12, 2018 at 06:44:49PM +0530, Abhishek Sahu wrote:
> * v2:
>
> 1. Address review comments in v1
> 2. Changed the license to SPDX
> 3. Changed commit messages for some of the patch having more detail
> 4. Removed event-based completion and changed transfer completion
> detection logic in interrupt handler
> 5. Removed dma_threshold and blk_mode_threshold from global structure
> 6. Improved determine mode logic for QUP v2 transfers
> 7. Fixed function comments
> 8. Fixed auto build test WARNING ‘idx' may be used uninitialized
> in this function
> 9. Renamed tx/rx_buf to tx/rx_cnt
>
> * v1:
>
> The current driver is failing in following test case
> 1. Handling of failure cases is not working in long run for BAM
> mode. It generates error message “bam-dma-engine 7884000.dma: Cannot
> free busy channel” sometimes.
> 2. Following I2C transfers are failing
> a. Single transfer with multiple read messages
> b. Single transfer with multiple read/write message with maximum
> allowed length per message (65K) in BAM mode
> c. Single transfer with write greater than 32 bytes in QUP v1 and
> write greater than 64 bytes in QUP v2 for non-DMA mode.
> 3. No handling is present for Block/FIFO interrupts. Any non-error
> interrupts are being treated as the transfer completion and then
> polling is being done for available/free bytes in FIFO.
>
> To fix all these issues, major code changes are required. This patch
> series fixes all the above issues and makes the driver interrupt based
> instead of polling based. After these changes, all the mentioned test
> cases are working properly.
>
> The code changes have been tested for QUP v1 (IPQ8064) and QUP
> v2 (IPQ8074) with sample application written over i2c-dev.
>
> Abhishek Sahu (13):
> i2c: qup: fix copyrights and update to SPDX identifier
> i2c: qup: fixed releasing dma without flush operation completion
> i2c: qup: minor code reorganization for use_dma
> i2c: qup: remove redundant variables for BAM SG count
> i2c: qup: schedule EOT and FLUSH tags at the end of transfer
> i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags
> i2c: qup: proper error handling for i2c error in BAM mode
> i2c: qup: use the complete transfer length to choose DMA mode
> i2c: qup: change completion timeout according to transfer length
> i2c: qup: fix buffer overflow for multiple msg of maximum xfer len
> i2c: qup: send NACK for last read sub transfers
> i2c: qup: reorganization of driver code to remove polling for qup v1
> i2c: qup: reorganization of driver code to remove polling for qup v2

Applied to for-next, thanks! Also thanks to the reviewers!

Attachments:

(No filename) (2.69 kB)
signature.asc (849.00 B)
Download all attachments

2018-03-26 04:44:59

by Abhishek Sahu

[permalink] [raw]

Subject: Re: [PATCH v2 00/13] Major code reorganization to make all i2c transfers working

On 2018-03-24 17:52, Wolfram Sang wrote:
> On Mon, Mar 12, 2018 at 06:44:49PM +0530, Abhishek Sahu wrote:
>> * v2:
>>
>> 1. Address review comments in v1
>> 2. Changed the license to SPDX
>> 3. Changed commit messages for some of the patch having more detail
>> 4. Removed event-based completion and changed transfer completion
>> detection logic in interrupt handler
>> 5. Removed dma_threshold and blk_mode_threshold from global structure
>> 6. Improved determine mode logic for QUP v2 transfers
>> 7. Fixed function comments
>> 8. Fixed auto build test WARNING ‘idx' may be used uninitialized
>> in this function
>> 9. Renamed tx/rx_buf to tx/rx_cnt
>>
>> * v1:
>>
>> The current driver is failing in following test case
>> 1. Handling of failure cases is not working in long run for BAM
>> mode. It generates error message “bam-dma-engine 7884000.dma:
>> Cannot
>> free busy channel” sometimes.
>> 2. Following I2C transfers are failing
>> a. Single transfer with multiple read messages
>> b. Single transfer with multiple read/write message with maximum
>> allowed length per message (65K) in BAM mode
>> c. Single transfer with write greater than 32 bytes in QUP v1 and
>> write greater than 64 bytes in QUP v2 for non-DMA mode.
>> 3. No handling is present for Block/FIFO interrupts. Any non-error
>> interrupts are being treated as the transfer completion and then
>> polling is being done for available/free bytes in FIFO.
>>
>> To fix all these issues, major code changes are required. This patch
>> series fixes all the above issues and makes the driver interrupt based
>> instead of polling based. After these changes, all the mentioned test
>> cases are working properly.
>>
>> The code changes have been tested for QUP v1 (IPQ8064) and QUP
>> v2 (IPQ8074) with sample application written over i2c-dev.
>>
>> Abhishek Sahu (13):
>> i2c: qup: fix copyrights and update to SPDX identifier
>> i2c: qup: fixed releasing dma without flush operation completion
>> i2c: qup: minor code reorganization for use_dma
>> i2c: qup: remove redundant variables for BAM SG count
>> i2c: qup: schedule EOT and FLUSH tags at the end of transfer
>> i2c: qup: fix the transfer length for BAM RX EOT FLUSH tags
>> i2c: qup: proper error handling for i2c error in BAM mode
>> i2c: qup: use the complete transfer length to choose DMA mode
>> i2c: qup: change completion timeout according to transfer length
>> i2c: qup: fix buffer overflow for multiple msg of maximum xfer len
>> i2c: qup: send NACK for last read sub transfers
>> i2c: qup: reorganization of driver code to remove polling for qup v1
>> i2c: qup: reorganization of driver code to remove polling for qup v2
>
> Applied to for-next, thanks! Also thanks to the reviewers!

Thanks Wolfram for your help in getting this big
patch series applied to for-next.

Thanks to Andy, Sricharan, Austin and other reviewers for
reviewing/testing the patches.

Regards,
Abhishek