| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e0009777-f185-e580-a1a2-13f283eb77ee@codeaurora.org>
Date: Tue, 20 Feb 2018 10:02:04 +0530
From: Sricharan R <sricharan@...eaurora.org>
To: Abhishek Sahu <absahu@...eaurora.org>
Cc: Andy Gross <andy.gross@...aro.org>,
Wolfram Sang <wsa@...-dreams.de>,
David Brown <david.brown@...aro.org>,
linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/12] i2c: qup: reorganization of driver code to remove
polling for qup v1
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@...eaurora.org>
>>> ---
>>> 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
Powered by blists - more mailing lists