Re: [PATCH 11/12] i2c: qup: reorganization of driver code to remove polling for qup v1

[Sricharan R]

Hi Abhishek,

On 2/19/2018 6:51 PM, Abhishek Sahu wrote:
> On 2018-02-16 13:14, Sricharan R wrote:
>> Hi Abhishek,
>>
>> On 2/3/2018 1:28 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_REQ.
>>>
>>> Because of above, i2c v1 transfers of size greater than 32 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. 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.
>>> 3. 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.
>>> 4. 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.
>>>
>>> Signed-off-by: Abhishek Sahu <absahu@xxxxxxxxxxxxxx>
>>> ---
>>> Âdrivers/i2c/busses/i2c-qup.c | 399 ++++++++++++++++++++++++++++---------------
>>> Â1 file changed, 257 insertions(+), 142 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>>> index edea3b9..af6c21a 100644
>>> --- a/drivers/i2c/busses/i2c-qup.c
>>> +++ b/drivers/i2c/busses/i2c-qup.c
>>> @@ -73,8 +73,11 @@
>>> Â#define QUP_IN_SVC_FLAGÂÂÂÂÂÂÂ BIT(9)
>>> Â#define QUP_MX_OUTPUT_DONEÂÂÂ BIT(10)
>>> Â#define QUP_MX_INPUT_DONEÂÂÂ BIT(11)
>>> +#define OUT_BLOCK_WRITE_REQÂÂÂ BIT(12)
>>> +#define IN_BLOCK_READ_REQÂÂÂ BIT(13)
>>>
>>> Â/* I2C mini core related values */
>>> +#define QUP_NO_INPUTÂÂÂÂÂÂÂ BIT(7)
>>> Â#define QUP_CLOCK_AUTO_GATEÂÂÂ BIT(13)
>>> Â#define I2C_MINI_COREÂÂÂÂÂÂÂ (2 << 8)
>>> Â#define I2C_N_VALÂÂÂÂÂÂÂ 15
>>> @@ -138,13 +141,51 @@
>>> Â#define DEFAULT_CLK_FREQ 100000
>>> Â#define DEFAULT_SRC_CLK 20000000
>>>
>>> +/* MAX_OUTPUT_DONE_FLAG has been received */
>>> +#define QUP_BLK_EVENT_TX_IRQ_DONEÂÂÂÂÂÂÂ BIT(0)
>>> +/* MAX_INPUT_DONE_FLAG has been received */
>>> +#define QUP_BLK_EVENT_RX_IRQ_DONEÂÂÂÂÂÂÂ BIT(1)
>>> +/* All the TX bytes have been written in TX FIFO */
>>> +#define QUP_BLK_EVENT_TX_DATA_DONEÂÂÂÂÂÂÂ BIT(2)
>>> +/* All the RX bytes have been read from RX FIFO */
>>> +#define QUP_BLK_EVENT_RX_DATA_DONEÂÂÂÂÂÂÂ BIT(3)
>>> +
>>> +/* All the required events to mark a QUP I2C TX transfer completed */
>>> +#define QUP_BLK_EVENT_TX_DONEÂÂÂÂÂÂÂ (QUP_BLK_EVENT_TX_IRQ_DONE | \
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ QUP_BLK_EVENT_TX_DATA_DONE)
>>> +/* All the required events to mark a QUP I2C RX transfer completed */
>>> +#define QUP_BLK_EVENT_RX_DONEÂÂÂÂÂÂÂ (QUP_BLK_EVENT_TX_DONE | \
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ QUP_BLK_EVENT_RX_IRQ_DONE | \
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ QUP_BLK_EVENT_RX_DATA_DONE)
>>> +
>>> +/*
>>> + * 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
>>> + * 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_free: number of free bytes in current QUP block write.
>>> + * fifo_available: number of available bytes in RX FIFO for current
>>> + *ÂÂÂÂÂÂÂÂÂÂ QUP block read
>>> + * 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
>>> + */
>>> Âstruct qup_i2c_block {
>>> -ÂÂÂ intÂÂÂ count;
>>> -ÂÂÂ intÂÂÂ pos;
>>> -ÂÂÂ intÂÂÂ tx_tag_len;
>>> -ÂÂÂ intÂÂÂ rx_tag_len;
>>> -ÂÂÂ intÂÂÂ data_len;
>>> -ÂÂÂ u8ÂÂÂ tags[6];
>>> +ÂÂÂ intÂÂÂÂÂÂÂ count;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ pos;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ tx_tag_len;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ rx_tag_len;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ data_len;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ total_tx_len;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ total_rx_len;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ tx_fifo_free;
>>> +ÂÂÂ intÂÂÂÂÂÂÂ fifo_available;
>>> +ÂÂÂ boolÂÂÂÂÂÂÂ is_tx_blk_mode;
>>> +ÂÂÂ boolÂÂÂÂÂÂÂ is_rx_blk_mode;
>>> +ÂÂÂ u8ÂÂÂÂÂÂÂ tags[6];
>>> Â};
>>>
>>> Âstruct qup_i2c_tag {
>>> @@ -187,6 +228,7 @@ struct qup_i2c_dev {
>>>
>>> ÂÂÂÂ /* To check if this is the last msg */
>>> ÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ is_last;
>>> +ÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ is_qup_v1;
>>>
>>> ÂÂÂÂ /* To configure when bus is in run state */
>>> ÂÂÂÂ intÂÂÂÂÂÂÂÂÂÂÂ config_run;
>>> @@ -195,6 +237,10 @@ struct qup_i2c_dev {
>>> ÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ is_dma;
>>> ÂÂÂÂ /* To check if the current transfer is using DMA */
>>> ÂÂÂÂ boolÂÂÂÂÂÂÂÂÂÂÂ use_dma;
>>> +ÂÂÂ /* Required events to mark QUP transfer as completed */
>>> +ÂÂÂ u32ÂÂÂÂÂÂÂÂÂÂÂ blk_events;
>>> +ÂÂÂ /* Already completed events in QUP transfer */
>>> +ÂÂÂ u32ÂÂÂÂÂÂÂÂÂÂÂ cur_blk_events;
>>> ÂÂÂÂ /* The threshold length above which DMA will be used */
>>> ÂÂÂÂ unsigned intÂÂÂÂÂÂÂ dma_threshold;
>>> ÂÂÂÂ unsigned intÂÂÂÂÂÂÂ max_xfer_sg_len;
>>> @@ -205,11 +251,18 @@ struct qup_i2c_dev {
>>> ÂÂÂÂ structÂÂÂÂÂÂÂÂÂÂÂ qup_i2c_bam btx;
>>>
>>> ÂÂÂÂ struct completionÂÂÂ xfer;
>>> +ÂÂÂ /* function to write data in tx fifo */
>>> +ÂÂÂ void (*write_tx_fifo)(struct qup_i2c_dev *qup);
>>> +ÂÂÂ /* function to read data from rx fifo */
>>> +ÂÂÂ void (*read_rx_fifo)(struct qup_i2c_dev *qup);
>>> +ÂÂÂ /* function to write tags in tx fifo for i2c read transfer */
>>> +ÂÂÂ void (*write_rx_tags)(struct qup_i2c_dev *qup);
>>> Â};
>>>
>>> Âstatic irqreturn_t qup_i2c_interrupt(int irq, void *dev)
>>> Â{
>>> ÂÂÂÂ struct qup_i2c_dev *qup = dev;
>>> +ÂÂÂ struct qup_i2c_block *blk = &qup->blk;
>>> ÂÂÂÂ u32 bus_err;
>>> ÂÂÂÂ u32 qup_err;
>>> ÂÂÂÂ u32 opflags;
>>> @@ -256,12 +309,54 @@ static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
>>> ÂÂÂÂÂÂÂÂ goto done;
>>> ÂÂÂÂ }
>>>
>>> -ÂÂÂ if (opflags & QUP_IN_SVC_FLAG)
>>> -ÂÂÂÂÂÂÂ writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
>>> +ÂÂÂ if (!qup->is_qup_v1)
>>> +ÂÂÂÂÂÂÂ goto done;
>>>
>>> -ÂÂÂ if (opflags & QUP_OUT_SVC_FLAG)
>>> +ÂÂÂ if (opflags & QUP_OUT_SVC_FLAG) {
>>> ÂÂÂÂÂÂÂÂ writel(QUP_OUT_SVC_FLAG, qup->base + QUP_OPERATIONAL);
>>>
>>> +ÂÂÂÂÂÂÂ /*
>>> +ÂÂÂÂÂÂÂÂ * Ideally, would like to check QUP_MAX_OUTPUT_DONE_FLAG.
>>> +ÂÂÂÂÂÂÂÂ * However, QUP_MAX_OUTPUT_DONE_FLAG is lagging behind
>>> +ÂÂÂÂÂÂÂÂ * QUP_OUTPUT_SERVICE_FLAG. The only reason for
>>> +ÂÂÂÂÂÂÂÂ * QUP_OUTPUT_SERVICE_FLAG to be set in FIFO mode is
>>> +ÂÂÂÂÂÂÂÂ * QUP_MAX_OUTPUT_DONE_FLAG condition. The code checking
>>> +ÂÂÂÂÂÂÂÂ * here QUP_OUTPUT_SERVICE_FLAG and assumes that
>>> +ÂÂÂÂÂÂÂÂ * QUP_MAX_OUTPUT_DONE_FLAG.
>>> +ÂÂÂÂÂÂÂÂ */
>>> +ÂÂÂÂÂÂÂ if (!blk->is_tx_blk_mode)
>>> +ÂÂÂÂÂÂÂÂÂÂÂ qup->cur_blk_events |= QUP_BLK_EVENT_TX_IRQ_DONE;
>>> +
>>> +ÂÂÂÂÂÂÂ if (opflags & OUT_BLOCK_WRITE_REQ) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ blk->tx_fifo_free += qup->out_blk_sz;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ if (qup->msg->flags & I2C_M_RD)
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ qup->write_rx_tags(qup);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ else
>>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ qup->write_tx_fifo(qup);
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ if (opflags & QUP_IN_SVC_FLAG) {
>>> +ÂÂÂÂÂÂÂ writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
>>> +
>>> +ÂÂÂÂÂÂÂ if (!blk->is_rx_blk_mode) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ blk->fifo_available += qup->in_fifo_sz;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ qup->read_rx_fifo(qup);
>>> +ÂÂÂÂÂÂÂ } else if (opflags & IN_BLOCK_READ_REQ) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ blk->fifo_available += qup->in_blk_sz;
>>> +ÂÂÂÂÂÂÂÂÂÂÂ qup->read_rx_fifo(qup);
>>> +ÂÂÂÂÂÂÂ }
>>> +ÂÂÂ }
>>> +
>>> +ÂÂÂ if (opflags & QUP_MX_OUTPUT_DONE)
>>> +ÂÂÂÂÂÂÂ qup->cur_blk_events |= QUP_BLK_EVENT_TX_IRQ_DONE;
>>> +
>>> +ÂÂÂ if (opflags & QUP_MX_INPUT_DONE)
>>> +ÂÂÂÂÂÂÂ qup->cur_blk_events |= QUP_BLK_EVENT_RX_IRQ_DONE;
>>> +
>>> +ÂÂÂ if (qup->cur_blk_events != qup->blk_events)
>>> +ÂÂÂÂÂÂÂ return IRQ_HANDLED;
>>
>> Â Is it correct that if we do a complete in above case, i mean
>> Â for TX -> based on QUP_MX_OUTPUT_DONE and for RX -> based on
>> Â QUP_MX_INPUT_DONE, would that simplify things by getting rid of
>> Â QUP_BLK_EVENT_RX/TX_DONE and QUP_BLK_EVENT_RX/TX_IRQ_DONE
>> Â altogether ?
> 
> ÂWe can get rid of QUP_BLK_EVENT_TX_DONE.
> ÂFor RX, the QUP_MX_INPUT_DONE will be triggered when QUP copies all
> Âthe data in FIFO from external i2c slave. So if 64 bytes read has been
> Âscheduled then following is the behaviour
> 
> ÂIRQ with QUP_MX_INPUT_DONE and IN_BLOCK_READ_REQ -> read 16 bytes
> ÂIRQ with IN_BLOCK_READ_REQ -> read next 16 bytes
> ÂIRQ with IN_BLOCK_READ_REQÂ -> read next 16 bytes
> ÂIRQ with IN_BLOCK_READ_REQ -> read last 16 bytes
> 
> ÂSo for RX, we can't trigger complete by checking QUP_BLK_EVENT_RX_DONE alone.
> ÂWe need to track the number of bytes read from FIFO. Instead of putting
> Âthis check, I am taking one extra event bit QUP_BLK_EVENT_RX_DONE which
> Âwill be set when all the bytes has been read.
> 
> ÂI am not sure if checking QUP_MX_INPUT_DONE will always work since
> Âthere may be some case, when for small transfers the QUP_MX_INPUT_DONE
> Âwill come before QUP_MX_OUTPUT_DONE so checking for both will work
> Âalways.

Looking in to the code and the above case,
  RX -> complete when the required len bytes are read from FIFO in to the msg buffer.
  TX -> complete just when QUP_MX_OUTPUT_DONE is set.

Tf this helps of getting rid of 3 of the above 4 flags tracking and all your stress/testing
continues to work then fine.

Regards,
 Sricharan

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