This series contain two patches, first is to optimize Mediatek I2C driver to use WRRD
if hardware support auto restart. Because auto restart will issue auto restart
interrupt, change to use WRRD can reduce interrupt latency. The second is to fix
multi transfer error in high speed mode. If hardware support auto restart, need driver
to send master code first.
Change in v2:
fix i2c checkpatch error.
Liguo Zhang (2):
i2c: mediatek: add i2c first write then read optimization
i2c: mediatek: fix i2c multi transfer issue in high speed mode
drivers/i2c/busses/i2c-mt65xx.c | 78 +++++++++++++++++++++++++++++++++++++----
1 file changed, 72 insertions(+), 6 deletions(-)
--
1.8.1.1.dirty
For platform with auto restart support, between every transfer,
i2c controller will trigger an interrupt and SW need to handle
it to start new transfer. When doing write-then-read transfer,
instead of restart mechanism, using WRRD mode to have controller
send both transfer in one request to reduce latency.
Signed-off-by: Liguo Zhang <[email protected]>
Reviewed-by: Eddie Huang <[email protected]>
---
drivers/i2c/busses/i2c-mt65xx.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 9b86716..dc4aac6 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -132,6 +132,7 @@ struct mtk_i2c_compatible {
unsigned char pmic_i2c: 1;
unsigned char dcm: 1;
unsigned char auto_restart: 1;
+ unsigned char aux_len_reg: 1;
};
struct mtk_i2c {
@@ -153,6 +154,7 @@ struct mtk_i2c {
enum mtk_trans_op op;
u16 timing_reg;
u16 high_speed_reg;
+ unsigned char auto_restart;
const struct mtk_i2c_compatible *dev_comp;
};
@@ -178,6 +180,7 @@ static const struct mtk_i2c_compatible mt6577_compat = {
.pmic_i2c = 0,
.dcm = 1,
.auto_restart = 0,
+ .aux_len_reg = 0,
};
static const struct mtk_i2c_compatible mt6589_compat = {
@@ -185,6 +188,7 @@ static const struct mtk_i2c_compatible mt6589_compat = {
.pmic_i2c = 1,
.dcm = 0,
.auto_restart = 0,
+ .aux_len_reg = 0,
};
static const struct mtk_i2c_compatible mt8173_compat = {
@@ -192,6 +196,7 @@ static const struct mtk_i2c_compatible mt8173_compat = {
.pmic_i2c = 0,
.dcm = 1,
.auto_restart = 1,
+ .aux_len_reg = 1,
};
static const struct of_device_id mtk_i2c_of_match[] = {
@@ -373,7 +378,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
i2c->irq_stat = 0;
- if (i2c->dev_comp->auto_restart)
+ if (i2c->auto_restart)
restart_flag = I2C_RS_TRANSFER;
reinit_completion(&i2c->msg_complete);
@@ -411,8 +416,14 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
/* Set transfer and transaction len */
if (i2c->op == I2C_MASTER_WRRD) {
- writew(msgs->len | ((msgs + 1)->len) << 8,
- i2c->base + OFFSET_TRANSFER_LEN);
+ if (i2c->dev_comp->aux_len_reg) {
+ writew(msgs->len, i2c->base + OFFSET_TRANSFER_LEN);
+ writew((msgs + 1)->len, i2c->base +
+ OFFSET_TRANSFER_LEN_AUX);
+ } else {
+ writew(msgs->len | ((msgs + 1)->len) << 8,
+ i2c->base + OFFSET_TRANSFER_LEN);
+ }
writew(I2C_WRRD_TRANAC_VALUE, i2c->base + OFFSET_TRANSAC_LEN);
} else {
writew(msgs->len, i2c->base + OFFSET_TRANSFER_LEN);
@@ -461,7 +472,7 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
writel(I2C_DMA_START_EN, i2c->pdmabase + OFFSET_EN);
- if (!i2c->dev_comp->auto_restart) {
+ if (!i2c->auto_restart) {
start_reg = I2C_TRANSAC_START;
} else {
start_reg = I2C_TRANSAC_START | I2C_RS_MUL_TRIG;
@@ -518,6 +529,16 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
if (ret)
return ret;
+ i2c->auto_restart = i2c->dev_comp->auto_restart;
+
+ /* checking if we can skip restart and optimize using WRRD mode */
+ if (i2c->auto_restart && num == 2) {
+ if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+ msgs[0].addr == msgs[1].addr) {
+ i2c->auto_restart = 0;
+ }
+ }
+
while (left_num--) {
if (!msgs->buf) {
dev_dbg(i2c->dev, "data buffer is NULL.\n");
@@ -530,7 +551,7 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
else
i2c->op = I2C_MASTER_WR;
- if (!i2c->dev_comp->auto_restart) {
+ if (!i2c->auto_restart) {
if (num > 1) {
/* combined two messages into one transaction */
i2c->op = I2C_MASTER_WRRD;
@@ -559,7 +580,7 @@ static irqreturn_t mtk_i2c_irq(int irqno, void *dev_id)
u16 restart_flag = 0;
u16 intr_stat;
- if (i2c->dev_comp->auto_restart)
+ if (i2c->auto_restart)
restart_flag = I2C_RS_TRANSFER;
intr_stat = readw(i2c->base + OFFSET_INTR_STAT);
--
1.8.1.1.dirty
For platform with auto restart support, when doing i2c multi transfer
in high speed, for example, doing write-then-read transfer, the master
code will occupy the first transfer, and the second transfer will be
the read transfer, the write transfer will be discarded. So we should
first send the master code, and then start i2c multi transfer.
Signed-off-by: Liguo Zhang <[email protected]>
Reviewed-by: Eddie Huang <[email protected]>
---
drivers/i2c/busses/i2c-mt65xx.c | 45 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index dc4aac6..249df86 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -53,6 +53,8 @@
#define I2C_FS_TIME_INIT_VALUE 0x1303
#define I2C_WRRD_TRANAC_VALUE 0x0002
#define I2C_RD_TRANAC_VALUE 0x0001
+#define I2C_TRAN_DEFAULT_VALUE 0x0001
+#define I2C_TRANAC_DEFAULT_VALUE 0x0001
#define I2C_DMA_CON_TX 0x0000
#define I2C_DMA_CON_RX 0x0001
@@ -365,6 +367,43 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk,
return 0;
}
+static int mtk_i2c_send_master_code(struct mtk_i2c *i2c)
+{
+ int ret = 0;
+
+ reinit_completion(&i2c->msg_complete);
+
+ writew(I2C_CONTROL_RS | I2C_CONTROL_ACKERR_DET_EN |
+ I2C_CONTROL_CLK_EXT_EN | I2C_CONTROL_DMA_EN,
+ i2c->base + OFFSET_CONTROL);
+
+ /* Clear interrupt status */
+ writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP | I2C_HS_NACKERR | I2C_ACKERR,
+ i2c->base + OFFSET_INTR_STAT);
+
+ /* Enable interrupt */
+ writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP, i2c->base +
+ OFFSET_INTR_MASK);
+
+ writew(I2C_TRAN_DEFAULT_VALUE, i2c->base + OFFSET_TRANSFER_LEN);
+ writew(I2C_TRANAC_DEFAULT_VALUE, i2c->base + OFFSET_TRANSAC_LEN);
+
+ writew(I2C_TRANSAC_START | I2C_RS_MUL_CNFG, i2c->base + OFFSET_START);
+
+ ret = wait_for_completion_timeout(&i2c->msg_complete,
+ i2c->adap.timeout);
+
+ completion_done(&i2c->msg_complete);
+
+ if (ret == 0) {
+ dev_dbg(i2c->dev, "send master code timeout.\n");
+ mtk_i2c_init_hw(i2c);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
int num, int left_num)
{
@@ -539,6 +578,12 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
}
}
+ if (i2c->auto_restart && i2c->speed_hz > 400000) {
+ ret = mtk_i2c_send_master_code(i2c);
+ if (ret)
+ return ret;
+ }
+
while (left_num--) {
if (!msgs->buf) {
dev_dbg(i2c->dev, "data buffer is NULL.\n");
--
1.8.1.1.dirty
On Mon, Nov 9, 2015 at 7:43 AM, Liguo Zhang <[email protected]> wrote:
> For platform with auto restart support, between every transfer,
> i2c controller will trigger an interrupt and SW need to handle
> it to start new transfer. When doing write-then-read transfer,
> instead of restart mechanism, using WRRD mode to have controller
> send both transfer in one request to reduce latency.
> @@ -518,6 +529,16 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> if (ret)
> return ret;
>
> + i2c->auto_restart = i2c->dev_comp->auto_restart;
> +
> + /* checking if we can skip restart and optimize using WRRD mode */
> + if (i2c->auto_restart && num == 2) {
> + if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
> + msgs[0].addr == msgs[1].addr) {
Nitpick (optional):
((msgs[0].flags & msgs[1].flags) & I2C_M_RD)
?
> + i2c->auto_restart = 0;
> + }
> + }
--
With Best Regards,
Andy Shevchenko
On Mon, Nov 9, 2015 at 10:25 PM, Andy Shevchenko
<[email protected]> wrote:
> On Mon, Nov 9, 2015 at 7:43 AM, Liguo Zhang <[email protected]> wrote:
>> For platform with auto restart support, between every transfer,
>> i2c controller will trigger an interrupt and SW need to handle
>> it to start new transfer. When doing write-then-read transfer,
>> instead of restart mechanism, using WRRD mode to have controller
>> send both transfer in one request to reduce latency.
>
>
>> @@ -518,6 +529,16 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
>> if (ret)
>> return ret;
>>
>> + i2c->auto_restart = i2c->dev_comp->auto_restart;
>> +
>> + /* checking if we can skip restart and optimize using WRRD mode */
>> + if (i2c->auto_restart && num == 2) {
>> + if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>> + msgs[0].addr == msgs[1].addr) {
>
> Nitpick (optional):
>
> ((msgs[0].flags & msgs[1].flags) & I2C_M_RD)
> ?
IMHO, this makes the code less readable.
Leave this to the compiler's optimizer.
-Dan
>
>> + i2c->auto_restart = 0;
>> + }
>> + }
>
> --
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Mon, 2015-11-09 at 16:25 +0200, Andy Shevchenko wrote:
> On Mon, Nov 9, 2015 at 7:43 AM, Liguo Zhang <[email protected]> wrote:
> > For platform with auto restart support, between every transfer,
> > i2c controller will trigger an interrupt and SW need to handle
> > it to start new transfer. When doing write-then-read transfer,
> > instead of restart mechanism, using WRRD mode to have controller
> > send both transfer in one request to reduce latency.
>
>
> > @@ -518,6 +529,16 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> > if (ret)
> > return ret;
> >
> > + i2c->auto_restart = i2c->dev_comp->auto_restart;
> > +
> > + /* checking if we can skip restart and optimize using WRRD mode */
> > + if (i2c->auto_restart && num == 2) {
> > + if (!(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
> > + msgs[0].addr == msgs[1].addr) {
>
> Nitpick (optional):
>
> ((msgs[0].flags & msgs[1].flags) & I2C_M_RD)
> ?
These 2 check for different conditions.
The original one check the first one must NOT set I2C_M_RD, but second
one must set I2C_M_RD.
Joe.C
On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <[email protected]> wrote:
> For platform with auto restart support, when doing i2c multi transfer
> in high speed, for example, doing write-then-read transfer, the master
> code will occupy the first transfer, and the second transfer will be
> the read transfer, the write transfer will be discarded. So we should
> first send the master code, and then start i2c multi transfer.
>
> Signed-off-by: Liguo Zhang <[email protected]>
> Reviewed-by: Eddie Huang <[email protected]>
> ---
> drivers/i2c/busses/i2c-mt65xx.c | 45 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index dc4aac6..249df86 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -53,6 +53,8 @@
> #define I2C_FS_TIME_INIT_VALUE 0x1303
> #define I2C_WRRD_TRANAC_VALUE 0x0002
> #define I2C_RD_TRANAC_VALUE 0x0001
> +#define I2C_TRAN_DEFAULT_VALUE 0x0001
> +#define I2C_TRANAC_DEFAULT_VALUE 0x0001
"TRAN" and "TRANAC" are not good names; this should be "TRANSFER_LEN"
and "TRANSAC", based on the names of the registers to which you write
these constants.
Furthermore, these are not "default" values, they are the transfer
length and number of transactions for sending the "master code", so:
#define I2C_TRANSFER_LEN_MASTER_CODE 0x0001
#define I2C_TRANSAC_LEN_MASTER_CODE 0x0001
Similarly, I think the "TRANAC" in I2C_WRRD_TRANAC_VALUE and
I2C_RD_TRANAC_VALUE should also be TRANSAC.
>
> #define I2C_DMA_CON_TX 0x0000
> #define I2C_DMA_CON_RX 0x0001
> @@ -365,6 +367,43 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk,
> return 0;
> }
>
> +static int mtk_i2c_send_master_code(struct mtk_i2c *i2c)
> +{
> + int ret = 0;
> +
> + reinit_completion(&i2c->msg_complete);
> +
> + writew(I2C_CONTROL_RS | I2C_CONTROL_ACKERR_DET_EN |
> + I2C_CONTROL_CLK_EXT_EN | I2C_CONTROL_DMA_EN,
> + i2c->base + OFFSET_CONTROL);
> +
> + /* Clear interrupt status */
> + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP | I2C_HS_NACKERR | I2C_ACKERR,
> + i2c->base + OFFSET_INTR_STAT);
> +
> + /* Enable interrupt */
> + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP, i2c->base +
> + OFFSET_INTR_MASK);
> +
> + writew(I2C_TRAN_DEFAULT_VALUE, i2c->base + OFFSET_TRANSFER_LEN);
> + writew(I2C_TRANAC_DEFAULT_VALUE, i2c->base + OFFSET_TRANSAC_LEN);
> +
> + writew(I2C_TRANSAC_START | I2C_RS_MUL_CNFG, i2c->base + OFFSET_START);
> +
> + ret = wait_for_completion_timeout(&i2c->msg_complete,
> + i2c->adap.timeout);
How does the hardware know that this transaction should be a "master code"?
Do you have to tell the hardware what value ('00001XXX') to use as the
master code?
The Master Code must be sent at <= 400 kHz, not the target clock. How
does the hardware know what rate to use?
When sending the master code, arbitration is supposed to occur, such
that only one winning master can proceed with the following high speed
transaction.
Where do you check that you won this arbitration?
If this is not implemented, adding a "TODO" would be helpful.
> +
> + completion_done(&i2c->msg_complete);
This completion_done() is only useful if you check the return value.
You should check it too, since we should only check for timeout if the
message hasn't completed.
> +
> + if (ret == 0) {
> + dev_dbg(i2c->dev, "send master code timeout.\n");
> + mtk_i2c_init_hw(i2c);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
> int num, int left_num)
> {
> @@ -539,6 +578,12 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> }
> }
>
> + if (i2c->auto_restart && i2c->speed_hz > 400000) {
Don't we need to send the master code for *every* HS transaction, not
just "auto_restart"?
"400000" => You already have a macro for this: MAX_FS_MODE_SPEED
> + ret = mtk_i2c_send_master_code(i2c);
> + if (ret)
> + return ret;
> + }
> +
> while (left_num--) {
> if (!msgs->buf) {
> dev_dbg(i2c->dev, "data buffer is NULL.\n");
> --
> 1.8.1.1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Sat, Nov 14, 2015 at 10:38:42PM +0800, Daniel Kurtz wrote:
> On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <[email protected]> wrote:
> > For platform with auto restart support, when doing i2c multi transfer
> > in high speed, for example, doing write-then-read transfer, the master
> > code will occupy the first transfer, and the second transfer will be
> > the read transfer, the write transfer will be discarded. So we should
> > first send the master code, and then start i2c multi transfer.
> >
> > Signed-off-by: Liguo Zhang <[email protected]>
> > Reviewed-by: Eddie Huang <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-mt65xx.c | 45 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > index dc4aac6..249df86 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > @@ -53,6 +53,8 @@
> > #define I2C_FS_TIME_INIT_VALUE 0x1303
> > #define I2C_WRRD_TRANAC_VALUE 0x0002
> > #define I2C_RD_TRANAC_VALUE 0x0001
> > +#define I2C_TRAN_DEFAULT_VALUE 0x0001
> > +#define I2C_TRANAC_DEFAULT_VALUE 0x0001
>
> "TRAN" and "TRANAC" are not good names; this should be "TRANSFER_LEN"
> and "TRANSAC", based on the names of the registers to which you write
> these constants.
>
> Furthermore, these are not "default" values, they are the transfer
> length and number of transactions for sending the "master code", so:
>
> #define I2C_TRANSFER_LEN_MASTER_CODE 0x0001
> #define I2C_TRANSAC_LEN_MASTER_CODE 0x0001
>
> Similarly, I think the "TRANAC" in I2C_WRRD_TRANAC_VALUE and
> I2C_RD_TRANAC_VALUE should also be TRANSAC.
>
> >
> > #define I2C_DMA_CON_TX 0x0000
> > #define I2C_DMA_CON_RX 0x0001
> > @@ -365,6 +367,43 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk,
> > return 0;
> > }
> >
> > +static int mtk_i2c_send_master_code(struct mtk_i2c *i2c)
> > +{
> > + int ret = 0;
> > +
> > + reinit_completion(&i2c->msg_complete);
> > +
> > + writew(I2C_CONTROL_RS | I2C_CONTROL_ACKERR_DET_EN |
> > + I2C_CONTROL_CLK_EXT_EN | I2C_CONTROL_DMA_EN,
> > + i2c->base + OFFSET_CONTROL);
> > +
> > + /* Clear interrupt status */
> > + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP | I2C_HS_NACKERR | I2C_ACKERR,
> > + i2c->base + OFFSET_INTR_STAT);
> > +
> > + /* Enable interrupt */
> > + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP, i2c->base +
> > + OFFSET_INTR_MASK);
> > +
> > + writew(I2C_TRAN_DEFAULT_VALUE, i2c->base + OFFSET_TRANSFER_LEN);
> > + writew(I2C_TRANAC_DEFAULT_VALUE, i2c->base + OFFSET_TRANSAC_LEN);
> > +
> > + writew(I2C_TRANSAC_START | I2C_RS_MUL_CNFG, i2c->base + OFFSET_START);
> > +
> > + ret = wait_for_completion_timeout(&i2c->msg_complete,
> > + i2c->adap.timeout);
>
> How does the hardware know that this transaction should be a "master code"?
> Do you have to tell the hardware what value ('00001XXX') to use as the
> master code?
> The Master Code must be sent at <= 400 kHz, not the target clock. How
> does the hardware know what rate to use?
> When sending the master code, arbitration is supposed to occur, such
> that only one winning master can proceed with the following high speed
> transaction.
> Where do you check that you won this arbitration?
> If this is not implemented, adding a "TODO" would be helpful.
>
> > +
> > + completion_done(&i2c->msg_complete);
>
> This completion_done() is only useful if you check the return value.
> You should check it too, since we should only check for timeout if the
> message hasn't completed.
>
> > +
> > + if (ret == 0) {
> > + dev_dbg(i2c->dev, "send master code timeout.\n");
> > + mtk_i2c_init_hw(i2c);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
> > int num, int left_num)
> > {
> > @@ -539,6 +578,12 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> > }
> > }
> >
> > + if (i2c->auto_restart && i2c->speed_hz > 400000) {
>
> Don't we need to send the master code for *every* HS transaction, not
> just "auto_restart"?
>
> "400000" => You already have a macro for this: MAX_FS_MODE_SPEED
Please address Daniel's comments and questions.
Thanks!
On Mon, Nov 09, 2015 at 01:43:58PM +0800, Liguo Zhang wrote:
> For platform with auto restart support, between every transfer,
> i2c controller will trigger an interrupt and SW need to handle
> it to start new transfer. When doing write-then-read transfer,
> instead of restart mechanism, using WRRD mode to have controller
> send both transfer in one request to reduce latency.
>
> Signed-off-by: Liguo Zhang <[email protected]>
> Reviewed-by: Eddie Huang <[email protected]>
Applied to for-next, thanks!
Dear Daniel,
1. I will rename "TRAN" & "TRANAC" & "I2C_TRAN_DEFAULT_VALUE" & "I2C_TRAN_DEFAULT_VALUE".
2. How does the hardware know that this transaction should be a "master code"?
When we set i2c speed > 400K , the hardware will send master code. We have a register to configure master code，and usually we use the default value "00001000".
How does the hardware know what rate to use?
The i2c speed is defined in dts, and we will set the i2c speed in i2c register, so HW will know the rate to use.
When sending the master code, arbitration is supposed to occur, such that only one winning master can proceed with the following high speed transaction.
Where do you check that you won this arbitration?
Our i2c driver now only support one master.
3. I will delete the completion_done() function.
4. Don't we need to send the master code for *every* HS transaction, not just "auto_restart"?
When we don't use auto restart, we also need to send the master code, the master code is sent by HW automatically.
I am improving the master code section when use auto restart, and this will be released in the next patch.
"400000" => You already have a macro for this: MAX_FS_MODE_SPEED
Yes, I will use MAX_FS_MODE_SPEED.
Thanks!
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Daniel Kurtz
Sent: 2015年11月14日 22:39
To: Liguo Zhang (张立国)
Cc: Wolfram Sang; srv_heupstream; Matthias Brugger; Eddie Huang (黃智傑); Xudong Chen (陈旭东); Sascha Hauer; Linux I2C; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v2 2/2] i2c: mediatek: fix i2c multi transfer issue in high speed mode
On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <[email protected]> wrote:
> For platform with auto restart support, when doing i2c multi transfer
> in high speed, for example, doing write-then-read transfer, the master
> code will occupy the first transfer, and the second transfer will be
> the read transfer, the write transfer will be discarded. So we should
> first send the master code, and then start i2c multi transfer.
>
> Signed-off-by: Liguo Zhang <[email protected]>
> Reviewed-by: Eddie Huang <[email protected]>
> ---
> drivers/i2c/busses/i2c-mt65xx.c | 45
> +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> b/drivers/i2c/busses/i2c-mt65xx.c index dc4aac6..249df86 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> @@ -53,6 +53,8 @@
> #define I2C_FS_TIME_INIT_VALUE 0x1303
> #define I2C_WRRD_TRANAC_VALUE 0x0002
> #define I2C_RD_TRANAC_VALUE 0x0001
> +#define I2C_TRAN_DEFAULT_VALUE 0x0001
> +#define I2C_TRAN_DEFAULT_VALUE 0x0001
"TRAN" and "TRANAC" are not good names; this should be "TRANSFER_LEN"
and "TRANSAC", based on the names of the registers to which you write these constants.
Furthermore, these are not "default" values, they are the transfer length and number of transactions for sending the "master code", so:
#define I2C_TRANSFER_LEN_MASTER_CODE 0x0001
#define I2C_TRANSAC_LEN_MASTER_CODE 0x0001
Similarly, I think the "TRANAC" in I2C_WRRD_TRANAC_VALUE and I2C_RD_TRANAC_VALUE should also be TRANSAC.
>
> #define I2C_DMA_CON_TX 0x0000
> #define I2C_DMA_CON_RX 0x0001
> @@ -365,6 +367,43 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk,
> return 0;
> }
>
> +static int mtk_i2c_send_master_code(struct mtk_i2c *i2c) {
> + int ret = 0;
> +
> + reinit_completion(&i2c->msg_complete);
> +
> + writew(I2C_CONTROL_RS | I2C_CONTROL_ACKERR_DET_EN |
> + I2C_CONTROL_CLK_EXT_EN | I2C_CONTROL_DMA_EN,
> + i2c->base + OFFSET_CONTROL);
> +
> + /* Clear interrupt status */
> + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP | I2C_HS_NACKERR | I2C_ACKERR,
> + i2c->base + OFFSET_INTR_STAT);
> +
> + /* Enable interrupt */
> + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP, i2c->base +
> + OFFSET_INTR_MASK);
> +
> + writew(I2C_TRAN_DEFAULT_VALUE, i2c->base + OFFSET_TRANSFER_LEN);
> + writew(I2C_TRANAC_DEFAULT_VALUE, i2c->base +
> + OFFSET_TRANSAC_LEN);
> +
> + writew(I2C_TRANSAC_START | I2C_RS_MUL_CNFG, i2c->base +
> + OFFSET_START);
> +
> + ret = wait_for_completion_timeout(&i2c->msg_complete,
> + i2c->adap.timeout);
How does the hardware know that this transaction should be a "master code"?
Do you have to tell the hardware what value ('00001XXX') to use as the master code?
The Master Code must be sent at <= 400 kHz, not the target clock. How does the hardware know what rate to use?
When sending the master code, arbitration is supposed to occur, such that only one winning master can proceed with the following high speed transaction.
Where do you check that you won this arbitration?
If this is not implemented, adding a "TODO" would be helpful.
> +
> + completion_done(&i2c->msg_complete);
This completion_done() is only useful if you check the return value.
You should check it too, since we should only check for timeout if the message hasn't completed.
> +
> + if (ret == 0) {
> + dev_dbg(i2c->dev, "send master code timeout.\n");
> + mtk_i2c_init_hw(i2c);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
> +
> static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
> int num, int left_num) { @@ -539,6
> +578,12 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> }
> }
>
> + if (i2c->auto_restart && i2c->speed_hz > 400000) {
Don't we need to send the master code for *every* HS transaction, not just "auto_restart"?
"400000" => You already have a macro for this: MAX_FS_MODE_SPEED
> + ret = mtk_i2c_send_master_code(i2c);
> + if (ret)
> + return ret;
> + }
> +
> while (left_num--) {
> if (!msgs->buf) {
> dev_dbg(i2c->dev, "data buffer is NULL.\n");
> --
> 1.8.1.1.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Sat, 2015-11-14 at 22:38 +0800, Daniel Kurtz wrote:
> On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <[email protected]> wrote:
> > For platform with auto restart support, when doing i2c multi transfer
> > in high speed, for example, doing write-then-read transfer, the master
> > code will occupy the first transfer, and the second transfer will be
> > the read transfer, the write transfer will be discarded. So we should
> > first send the master code, and then start i2c multi transfer.
> >
> > Signed-off-by: Liguo Zhang <[email protected]>
> > Reviewed-by: Eddie Huang <[email protected]>
> > ---
> > drivers/i2c/busses/i2c-mt65xx.c | 45 +++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > index dc4aac6..249df86 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > @@ -53,6 +53,8 @@
> > #define I2C_FS_TIME_INIT_VALUE 0x1303
> > #define I2C_WRRD_TRANAC_VALUE 0x0002
> > #define I2C_RD_TRANAC_VALUE 0x0001
> > +#define I2C_TRAN_DEFAULT_VALUE 0x0001
> > +#define I2C_TRANAC_DEFAULT_VALUE 0x0001
>
> "TRAN" and "TRANAC" are not good names; this should be "TRANSFER_LEN"
> and "TRANSAC", based on the names of the registers to which you write
> these constants.
>
> Furthermore, these are not "default" values, they are the transfer
> length and number of transactions for sending the "master code", so:
>
> #define I2C_TRANSFER_LEN_MASTER_CODE 0x0001
> #define I2C_TRANSAC_LEN_MASTER_CODE 0x0001
>
> Similarly, I think the "TRANAC" in I2C_WRRD_TRANAC_VALUE and
> I2C_RD_TRANAC_VALUE should also be TRANSAC.
>
Yes, I will follow your advice.
> >
> > #define I2C_DMA_CON_TX 0x0000
> > #define I2C_DMA_CON_RX 0x0001
> > @@ -365,6 +367,43 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk,
> > return 0;
> > }
> >
> > +static int mtk_i2c_send_master_code(struct mtk_i2c *i2c)
> > +{
> > + int ret = 0;
> > +
> > + reinit_completion(&i2c->msg_complete);
> > +
> > + writew(I2C_CONTROL_RS | I2C_CONTROL_ACKERR_DET_EN |
> > + I2C_CONTROL_CLK_EXT_EN | I2C_CONTROL_DMA_EN,
> > + i2c->base + OFFSET_CONTROL);
> > +
> > + /* Clear interrupt status */
> > + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP | I2C_HS_NACKERR | I2C_ACKERR,
> > + i2c->base + OFFSET_INTR_STAT);
> > +
> > + /* Enable interrupt */
> > + writew(I2C_RS_TRANSFER | I2C_TRANSAC_COMP, i2c->base +
> > + OFFSET_INTR_MASK);
> > +
> > + writew(I2C_TRAN_DEFAULT_VALUE, i2c->base + OFFSET_TRANSFER_LEN);
> > + writew(I2C_TRANAC_DEFAULT_VALUE, i2c->base + OFFSET_TRANSAC_LEN);
> > +
> > + writew(I2C_TRANSAC_START | I2C_RS_MUL_CNFG, i2c->base + OFFSET_START);
> > +
> > + ret = wait_for_completion_timeout(&i2c->msg_complete,
> > + i2c->adap.timeout);
>
> How does the hardware know that this transaction should be a "master code"?
> Do you have to tell the hardware what value ('00001XXX') to use as the
> master code?
When we set i2c speed > 400K , the hardware will send master code. We
have a register to configure master code，and usually we use the default
value "00001000".
> The Master Code must be sent at <= 400 kHz, not the target clock. How
> does the hardware know what rate to use?
The i2c speed is defined in dts, and we will set the i2c speed in i2c
register, so HW will know the rate to use.
> When sending the master code, arbitration is supposed to occur, such
> that only one winning master can proceed with the following high speed
> transaction.
> Where do you check that you won this arbitration?
> If this is not implemented, adding a "TODO" would be helpful.
>
Our i2c driver now only support one master.
> > +
> > + completion_done(&i2c->msg_complete);
>
> This completion_done() is only useful if you check the return value.
> You should check it too, since we should only check for timeout if the
> message hasn't completed.
>
I will delete the completion_done() function.
> > +
> > + if (ret == 0) {
> > + dev_dbg(i2c->dev, "send master code timeout.\n");
> > + mtk_i2c_init_hw(i2c);
> > + return -ETIMEDOUT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
> > int num, int left_num)
> > {
> > @@ -539,6 +578,12 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
> > }
> > }
> >
> > + if (i2c->auto_restart && i2c->speed_hz > 400000) {
>
> Don't we need to send the master code for *every* HS transaction, not
> just "auto_restart"?
>
When we don't use auto restart, we also need to send the master code,
the master code is sent by HW automatically.
I am improving the master code section when use auto restart, and this
will be released in the next patch.
> "400000" => You already have a macro for this: MAX_FS_MODE_SPEED
>
Yes, I will use MAX_FS_MODE_SPEED.
> > + ret = mtk_i2c_send_master_code(i2c);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > while (left_num--) {
> > if (!msgs->buf) {
> > dev_dbg(i2c->dev, "data buffer is NULL.\n");
> > --
> > 1.8.1.1.dirty
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/