Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757149AbbDPJjW (ORCPT ); Thu, 16 Apr 2015 05:39:22 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:44999 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756874AbbDPJjM (ORCPT ); Thu, 16 Apr 2015 05:39:12 -0400 Message-ID: <552F8339.20300@codeaurora.org> Date: Thu, 16 Apr 2015 15:09:05 +0530 From: Sricharan R User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: "Ivan T. Ivanov" CC: devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org, galak@codeaurora.org, linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org, agross@codeaurora.org, dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH V3 2/6] i2c: qup: Add V2 tags support References: <1428736145-18361-1-git-send-email-sricharan@codeaurora.org> <1428736145-18361-3-git-send-email-sricharan@codeaurora.org> <1429024595.16939.5.camel@mm-sol.com> <552E07B5.5080605@codeaurora.org> <1429087776.30601.1.camel@mm-sol.com> <552E7962.40606@codeaurora.org> <1429173395.26621.5.camel@mm-sol.com> In-Reply-To: <1429173395.26621.5.camel@mm-sol.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7271 Lines: 177 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/