Re: [PATCH V3 2/6] i2c: qup: Add V2 tags support
From: Sricharan R
Date: Thu Apr 16 2015 - 05:39:24 EST
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/