Message-ID: <552F8339.20300@codeaurora.org>
Date:	Thu, 16 Apr 2015 15:09:05 +0530
From:	Sricharan R <sricharan@...eaurora.org>
To:	"Ivan T. Ivanov" <iivanov@...sol.com>
CC:	devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
	galak@...eaurora.org, linux-kernel@...r.kernel.org,
	linux-i2c@...r.kernel.org, agross@...eaurora.org,
	dmaengine@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH V3 2/6] i2c: qup: Add V2 tags support

Hi Ivan,

On 04/16/2015 02:06 PM, Ivan T. Ivanov wrote:
>
> Hi Sricharan,
>
> On Wed, 2015-04-15 at 20:14 +0530, Sricharan R wrote:
>>
>>>>>>
>>>>>> +#define QUP_I2C_MX_CONFIG_DURING_RUN   BIT(31)
>>>>>
>>>>> Could you explain what is this for?
>>>>>
>>>>      This is a new feature in the V2 version of the controller,
>>>>      to support multiple i2c sub transfers without having
>>>>      a 'stop' bit in-between. Without this the i2c controller
>>>>      inserts a 'stop' on the bus when the WR/RD count registers
>>>>      reaches zero and are configured for the next transfer. So setting
>>>>      this bit when the controller is in 'RUN' state, avoids sending the
>>>>      'stop' during RUN state.
>>>>      I can add this comment in the patch.
>>>
>>> And how v2 of this patch was worked if you introduce this bit now?
>>> Bit is also not used by downstream driver, AFICS?
>>>
>> The one of the reason for this and the bam support patches in
>> this series was to support multiple transfers of i2c_msgs without
>>    a 'stop' inbetween them. So without that the driver sends a 'stop'
>> between each sub-transfer.
>
> Are you saying that there is bug in the hardware? Please, could you
> point me to codeaurora driver, which is using this bit?
>
   The controller was not supporting this 'no-stop' feature by default
   and not sure whether to call it a 'bug' or 'missing feature', which
   it supports now anyway. Regarding the internal driver, it was
   trying to coalesce the writes (if they are to same address) by
   configuring the WR_CNT register to the sum of msg->len of the
   consecutive sub-transfers. This is no more needed with this 'feature'.
>
>
> -static void qup_i2c_set_write_mode(struct qup_i2c_dev *qup, struct i2c_msg *msg)
>>>>>> +static void qup_i2c_set_write_mode(struct qup_i2c_dev *qup, struct i2c_msg *msg,
>>>>>> +                                                       int run)
>>>>>
>>>>> And 'run' stands for?
>>>>     'run' just says whether the controller is in 'RUN' or 'RESET' state.
>>>>      I can change it to is_run_st to make it clear.
>>>>>>     {
>>>>>> -       /* Number of entries to shift out, including the start */
>>>>>> -       int total = msg->len + 1;
>>>>>> +       /* Total Number of entries to shift out, including the tags */
>>>>>> +       int total;
>>>>>> +
>>>>>> +       if (qup->use_v2_tags) {
>>>>>> +               total = msg->len + qup->tx_tag_len;
>>>>>> +               if (run)
>>>>>> +                       total |= QUP_I2C_MX_CONFIG_DURING_RUN;
>>>>>
>>>>> What?
>>>>>
>>>>         This means, if the controller is in 'RUN' state, for
>>>>         reconfiguring the RD/WR counts this bit has to be set to avoid
>>>>         'stop' bits.
>>>
>>> I don't buy it, sorry. Problem with v1 of the tags is that controller
>>> can not read more than 256 bytes without automatically generate STOP
>>> at the end, because transfer length specified with QUP_TAG_REC tag
>>> is 8 bits wide. There is no limitation for writes with v1 tags,
>>> because controller is explicitly instructed when to send out STOP.
>>>
>> correct till this.
>>
>>> For v2 of the tags, writes behaves more or less the same, and the
>>> good news are that there is new read tag QUP_TAG_V2_DATARD, which
>>> did't generate STOP when specified amount of data is read, still
>>> up to 256 bytes in chunk. Read transfers with size more than 256
>>> could be formed in following way:
>>>
>>> V2_START | Slave Address | V2_DATARD | countx | ... | V2_DATARD_STOP | county.
>>>
>>    The above is true for a single subtransfer for reading/writing
>> more than > 256 bytes. But for doing WRITE, READ kind of sub
>>    transfers once the first sub transfer (write) is over, and
>>    while re-configuring the _COUNT registers for the next transfers,
>> 'a stop' between is inserted.
>
>  From controller itself or driver?
>
  controller itself.

>>>>>> +static void qup_i2c_issue_xfer_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg)
>>>>>> +{
>>>>>> +       u32 data_len = 0, tag_len;
>>>>>> +
>>>>>> +       tag_len = qup->blk.block_tag_len[qup->blk.block_pos];
>>>>>> +
>>>>>> +       if (!(msg->flags & I2C_M_RD))
>>>>>> +               data_len = qup->blk.block_data_len[qup->blk.block_pos];
>>>>>> +
>>>>>> +       qup_i2c_send_data(qup, tag_len, qup->tags, data_len, msg->buf);
>>>>>
>>>>> This assumes that writes are up to 256 bytes, and tags and data blocks
>>>>> are completely written to FIFO buffer, right?
>>>>>
>>>>     Yes, since we send/read data in blocks (256 bytes).
>>>
>>> How deep is the FIFO? Is it guaranteed that "the whole" write
>>> or read data, including tags will fit in.
>>>
>>    Write/read fifo functions (also for V1) currently wait for the
>>     fifo full and empty flags conditions.
>
> I will say that this is true for v1, but not for v2,
> because the way of how FIFO is filled in v2.
>
   fifo is filled using the same 'flags' in both v1 and v2.
   The difference is the way tags and data are assembled in the
   output. But as i said, it can be improved atleast in v2 easily
   (can be done in v1 also, but is not something required in this
   patch) and i will change that in next version.

>>>>>> +static int qup_i2c_write_one(struct qup_i2c_dev *qup, struct i2c_msg *msg,
>>>>>> +                                                               int run, int last)
>>>>>>     {
>>>>>>            unsigned long left;
>>>>>>            int ret;
>>>>>> @@ -329,13 +501,20 @@ static int qup_i2c_write_one(struct qup_i2c_dev *qup, struct
>>>>>> i2c_msg *msg)
>>>>>>            qup->msg = msg;
>>>>>>            qup->pos = 0;
>>>>>>
>>>>>> +       if (qup->use_v2_tags)
>>>>>> +               qup_i2c_create_tag_v2(qup, msg, last);
>>>>>> +       else
>>>>>> +               qup->blk.blocks = 0;
>>>>>> +
>>>>>>            enable_irq(qup->irq);
>>>>>>
>>>>>> -       qup_i2c_set_write_mode(qup, msg);
>>>>>> +       qup_i2c_set_write_mode(qup, msg, run);
>>>>>>
>>>>>> -       ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
>>>>>> -       if (ret)
>>>>>> -               goto err;
>>>>>> +       if (!run) {
>>>>>> +               ret = qup_i2c_change_state(qup, QUP_RUN_STATE);
>>>>>
>>>>> To run away, or not?
>>>>>
>>>>      Means, if the controller is not in RUN state, put it in to 'RUN'
>>>>      state.
>>>
>>> And what is the problem if controller is put in PAUSED state, FIFO
>>> filled with data and then RUN again, like in v2 of this patch?
>>>
>>    This function is not entered with controller in PAUSED state
>>    Only in Reset state (for the first transfer) and Run state for
>>    the subsequent sub-transfers. The reason for having this 'run'
>>    variable was that while using the lock-unlock feature, the
>>    controller should not be put in to run-reset-run state
>>    in-between transfers.
>
> Lets see how it will look, when new qup_i2c_write_one_v2 is introduced :-)
>
ok.

Regards,
  Sricharan
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists