diff mbox

[v2,2/2] i2c: mediatek: fix i2c multi transfer issue in high speed mode

Message ID 1447047839-5223-3-git-send-email-liguo.zhang@mediatek.com
State Changes Requested
Headers show

Commit Message

Liguo Zhang Nov. 9, 2015, 5:43 a.m. UTC 
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 <liguo.zhang@mediatek.com>
Reviewed-by: Eddie Huang <eddie.huang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 45 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

Comments

Daniel Kurtz Nov. 14, 2015, 2:38 p.m. UTC | #1 
On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <liguo.zhang@mediatek.com> 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 <liguo.zhang@mediatek.com>
> Reviewed-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
>  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 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang Dec. 1, 2015, 12:54 a.m. UTC | #2 
On Sat, Nov 14, 2015 at 10:38:42PM +0800, Daniel Kurtz wrote:
> On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <liguo.zhang@mediatek.com> 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 <liguo.zhang@mediatek.com>
> > Reviewed-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> >  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!
Liguo Zhang Dec. 1, 2015, 11:24 a.m. UTC | #3 
RGVhciBEYW5pZWwsDQoNCjEuIEkgd2lsbCByZW5hbWUgIlRSQU4iICYgIlRSQU5BQyIgJiAiSTJD
X1RSQU5fREVGQVVMVF9WQUxVRSIgJiAiSTJDX1RSQU5fREVGQVVMVF9WQUxVRSIuDQoNCjIuIEhv
dyBkb2VzIHRoZSBoYXJkd2FyZSBrbm93IHRoYXQgdGhpcyB0cmFuc2FjdGlvbiBzaG91bGQgYmUg
YSAibWFzdGVyIGNvZGUiPw0KV2hlbiB3ZSBzZXQgaTJjIHNwZWVkID4gNDAwSyAsIHRoZSBoYXJk
d2FyZSB3aWxsIHNlbmQgbWFzdGVyIGNvZGUuIFdlIGhhdmUgYSByZWdpc3RlciB0byBjb25maWd1
cmUgbWFzdGVyIGNvZGXvvIxhbmQgdXN1YWxseSB3ZSB1c2UgdGhlIGRlZmF1bHQgdmFsdWUgIjAw
MDAxMDAwIi4NCg0KSG93IGRvZXMgdGhlIGhhcmR3YXJlIGtub3cgd2hhdCByYXRlIHRvIHVzZT8N
ClRoZSBpMmMgc3BlZWQgaXMgZGVmaW5lZCBpbiBkdHMsIGFuZCB3ZSB3aWxsIHNldCB0aGUgaTJj
IHNwZWVkIGluIGkyYyByZWdpc3Rlciwgc28gSFcgd2lsbCBrbm93IHRoZSByYXRlIHRvIHVzZS4N
Cg0KV2hlbiBzZW5kaW5nIHRoZSBtYXN0ZXIgY29kZSwgYXJiaXRyYXRpb24gaXMgc3VwcG9zZWQg
dG8gb2NjdXIsIHN1Y2ggdGhhdCBvbmx5IG9uZSB3aW5uaW5nIG1hc3RlciBjYW4gcHJvY2VlZCB3
aXRoIHRoZSBmb2xsb3dpbmcgaGlnaCBzcGVlZCB0cmFuc2FjdGlvbi4NCldoZXJlIGRvIHlvdSBj
aGVjayB0aGF0IHlvdSB3b24gdGhpcyBhcmJpdHJhdGlvbj8NCk91ciBpMmMgZHJpdmVyIG5vdyBv
bmx5IHN1cHBvcnQgb25lIG1hc3Rlci4NCg0KMy4gSSB3aWxsIGRlbGV0ZSB0aGUgY29tcGxldGlv
bl9kb25lKCkgZnVuY3Rpb24uDQoNCjQuIERvbid0IHdlIG5lZWQgdG8gc2VuZCB0aGUgbWFzdGVy
IGNvZGUgZm9yICpldmVyeSogSFMgdHJhbnNhY3Rpb24sIG5vdCBqdXN0ICJhdXRvX3Jlc3RhcnQi
Pw0KV2hlbiB3ZSBkb24ndCB1c2UgYXV0byByZXN0YXJ0LCB3ZSBhbHNvIG5lZWQgdG8gc2VuZCB0
aGUgbWFzdGVyIGNvZGUsIHRoZSBtYXN0ZXIgY29kZSBpcyBzZW50IGJ5IEhXIGF1dG9tYXRpY2Fs
bHkuDQpJIGFtIGltcHJvdmluZyB0aGUgbWFzdGVyIGNvZGUgc2VjdGlvbiB3aGVuIHVzZSBhdXRv
IHJlc3RhcnQsIGFuZCB0aGlzIHdpbGwgYmUgcmVsZWFzZWQgaW4gdGhlIG5leHQgcGF0Y2guDQoN
CiI0MDAwMDAiID0+IFlvdSBhbHJlYWR5IGhhdmUgYSBtYWNybyBmb3IgdGhpczogTUFYX0ZTX01P
REVfU1BFRUQNCg0KWWVzLCBJIHdpbGwgdXNlIE1BWF9GU19NT0RFX1NQRUVELg0KDQpUaGFua3Mh
DQoNCi0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQpGcm9tOiBkamt1cnR6QGdvb2dsZS5jb20g
W21haWx0bzpkamt1cnR6QGdvb2dsZS5jb21dIE9uIEJlaGFsZiBPZiBEYW5pZWwgS3VydHoNClNl
bnQ6IDIwMTXlubQxMeaciDE05pelIDIyOjM5DQpUbzogTGlndW8gWmhhbmcgKOW8oOeri+WbvSkN
CkNjOiBXb2xmcmFtIFNhbmc7IHNydl9oZXVwc3RyZWFtOyBNYXR0aGlhcyBCcnVnZ2VyOyBFZGRp
ZSBIdWFuZyAo6buD5pm65YKRKTsgWHVkb25nIENoZW4gKOmZiOaXreS4nCk7IFNhc2NoYSBIYXVl
cjsgTGludXggSTJDOyBsaW51eC1rZXJuZWxAdmdlci5rZXJuZWwub3JnOyBsaW51eC1hcm0ta2Vy
bmVsQGxpc3RzLmluZnJhZGVhZC5vcmc7IGxpbnV4LW1lZGlhdGVrQGxpc3RzLmluZnJhZGVhZC5v
cmcNClN1YmplY3Q6IFJlOiBbUEFUQ0ggdjIgMi8yXSBpMmM6IG1lZGlhdGVrOiBmaXggaTJjIG11
bHRpIHRyYW5zZmVyIGlzc3VlIGluIGhpZ2ggc3BlZWQgbW9kZQ0KDQpPbiBNb24sIE5vdiA5LCAy
MDE1IGF0IDE6NDMgUE0sIExpZ3VvIFpoYW5nIDxsaWd1by56aGFuZ0BtZWRpYXRlay5jb20+IHdy
b3RlOg0KPiBGb3IgcGxhdGZvcm0gd2l0aCBhdXRvIHJlc3RhcnQgc3VwcG9ydCwgd2hlbiBkb2lu
ZyBpMmMgbXVsdGkgdHJhbnNmZXIgDQo+IGluIGhpZ2ggc3BlZWQsIGZvciBleGFtcGxlLCBkb2lu
ZyB3cml0ZS10aGVuLXJlYWQgdHJhbnNmZXIsIHRoZSBtYXN0ZXIgDQo+IGNvZGUgd2lsbCBvY2N1
cHkgdGhlIGZpcnN0IHRyYW5zZmVyLCBhbmQgdGhlIHNlY29uZCB0cmFuc2ZlciB3aWxsIGJlIA0K
PiB0aGUgcmVhZCB0cmFuc2ZlciwgdGhlIHdyaXRlIHRyYW5zZmVyIHdpbGwgYmUgZGlzY2FyZGVk
LiBTbyB3ZSBzaG91bGQgDQo+IGZpcnN0IHNlbmQgdGhlIG1hc3RlciBjb2RlLCBhbmQgdGhlbiBz
dGFydCBpMmMgbXVsdGkgdHJhbnNmZXIuDQo+DQo+IFNpZ25lZC1vZmYtYnk6IExpZ3VvIFpoYW5n
IDxsaWd1by56aGFuZ0BtZWRpYXRlay5jb20+DQo+IFJldmlld2VkLWJ5OiBFZGRpZSBIdWFuZyA8
ZWRkaWUuaHVhbmdAbWVkaWF0ZWsuY29tPg0KPiAtLS0NCj4gIGRyaXZlcnMvaTJjL2J1c3Nlcy9p
MmMtbXQ2NXh4LmMgfCA0NSANCj4gKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr
KysrKysNCj4gIDEgZmlsZSBjaGFuZ2VkLCA0NSBpbnNlcnRpb25zKCspDQo+DQo+IGRpZmYgLS1n
aXQgYS9kcml2ZXJzL2kyYy9idXNzZXMvaTJjLW10NjV4eC5jIA0KPiBiL2RyaXZlcnMvaTJjL2J1
c3Nlcy9pMmMtbXQ2NXh4LmMgaW5kZXggZGM0YWFjNi4uMjQ5ZGY4NiAxMDA2NDQNCj4gLS0tIGEv
ZHJpdmVycy9pMmMvYnVzc2VzL2kyYy1tdDY1eHguYw0KPiArKysgYi9kcml2ZXJzL2kyYy9idXNz
ZXMvaTJjLW10NjV4eC5jDQo+IEBAIC01Myw2ICs1Myw4IEBADQo+ICAjZGVmaW5lIEkyQ19GU19U
SU1FX0lOSVRfVkFMVUUgICAgICAgICAweDEzMDMNCj4gICNkZWZpbmUgSTJDX1dSUkRfVFJBTkFD
X1ZBTFVFICAgICAgICAgIDB4MDAwMg0KPiAgI2RlZmluZSBJMkNfUkRfVFJBTkFDX1ZBTFVFICAg
ICAgICAgICAgMHgwMDAxDQo+ICsjZGVmaW5lIEkyQ19UUkFOX0RFRkFVTFRfVkFMVUUgICAgICAg
ICAweDAwMDENCj4gKyNkZWZpbmUgSTJDX1RSQU5fREVGQVVMVF9WQUxVRSAgICAgICAweDAwMDEN
Cg0KIlRSQU4iIGFuZCAiVFJBTkFDIiBhcmUgbm90IGdvb2QgbmFtZXM7IHRoaXMgc2hvdWxkIGJl
ICJUUkFOU0ZFUl9MRU4iDQphbmQgIlRSQU5TQUMiLCBiYXNlZCBvbiB0aGUgbmFtZXMgb2YgdGhl
IHJlZ2lzdGVycyB0byB3aGljaCB5b3Ugd3JpdGUgdGhlc2UgY29uc3RhbnRzLg0KDQpGdXJ0aGVy
bW9yZSwgdGhlc2UgYXJlIG5vdCAiZGVmYXVsdCIgdmFsdWVzLCB0aGV5IGFyZSB0aGUgdHJhbnNm
ZXIgbGVuZ3RoIGFuZCBudW1iZXIgb2YgdHJhbnNhY3Rpb25zIGZvciBzZW5kaW5nIHRoZSAibWFz
dGVyIGNvZGUiLCBzbzoNCg0KI2RlZmluZSBJMkNfVFJBTlNGRVJfTEVOX01BU1RFUl9DT0RFICAg
ICAgICAgMHgwMDAxDQojZGVmaW5lIEkyQ19UUkFOU0FDX0xFTl9NQVNURVJfQ09ERSAgICAgICAw
eDAwMDENCg0KU2ltaWxhcmx5LCBJIHRoaW5rIHRoZSAiVFJBTkFDIiBpbiBJMkNfV1JSRF9UUkFO
QUNfVkFMVUUgYW5kIEkyQ19SRF9UUkFOQUNfVkFMVUUgc2hvdWxkIGFsc28gYmUgVFJBTlNBQy4N
Cg0KPg0KPiAgI2RlZmluZSBJMkNfRE1BX0NPTl9UWCAgICAgICAgICAgICAgICAgMHgwMDAwDQo+
ICAjZGVmaW5lIEkyQ19ETUFfQ09OX1JYICAgICAgICAgICAgICAgICAweDAwMDENCj4gQEAgLTM2
NSw2ICszNjcsNDMgQEAgc3RhdGljIGludCBtdGtfaTJjX3NldF9zcGVlZChzdHJ1Y3QgbXRrX2ky
YyAqaTJjLCB1bnNpZ25lZCBpbnQgcGFyZW50X2NsaywNCj4gICAgICAgICByZXR1cm4gMDsNCj4g
IH0NCj4NCj4gK3N0YXRpYyBpbnQgbXRrX2kyY19zZW5kX21hc3Rlcl9jb2RlKHN0cnVjdCBtdGtf
aTJjICppMmMpIHsNCj4gKyAgICAgICBpbnQgcmV0ID0gMDsNCj4gKw0KPiArICAgICAgIHJlaW5p
dF9jb21wbGV0aW9uKCZpMmMtPm1zZ19jb21wbGV0ZSk7DQo+ICsNCj4gKyAgICAgICB3cml0ZXco
STJDX0NPTlRST0xfUlMgfCBJMkNfQ09OVFJPTF9BQ0tFUlJfREVUX0VOIHwNCj4gKyAgICAgICAg
ICAgICAgSTJDX0NPTlRST0xfQ0xLX0VYVF9FTiB8IEkyQ19DT05UUk9MX0RNQV9FTiwNCj4gKyAg
ICAgICAgICAgICAgaTJjLT5iYXNlICsgT0ZGU0VUX0NPTlRST0wpOw0KPiArDQo+ICsgICAgICAg
LyogQ2xlYXIgaW50ZXJydXB0IHN0YXR1cyAqLw0KPiArICAgICAgIHdyaXRldyhJMkNfUlNfVFJB
TlNGRVIgfCBJMkNfVFJBTlNBQ19DT01QIHwgSTJDX0hTX05BQ0tFUlIgfCBJMkNfQUNLRVJSLA0K
PiArICAgICAgICAgICAgICBpMmMtPmJhc2UgKyBPRkZTRVRfSU5UUl9TVEFUKTsNCj4gKw0KPiAr
ICAgICAgIC8qIEVuYWJsZSBpbnRlcnJ1cHQgKi8NCj4gKyAgICAgICB3cml0ZXcoSTJDX1JTX1RS
QU5TRkVSIHwgSTJDX1RSQU5TQUNfQ09NUCwgaTJjLT5iYXNlICsNCj4gKyAgICAgICAgICAgICAg
T0ZGU0VUX0lOVFJfTUFTSyk7DQo+ICsNCj4gKyAgICAgICB3cml0ZXcoSTJDX1RSQU5fREVGQVVM
VF9WQUxVRSwgaTJjLT5iYXNlICsgT0ZGU0VUX1RSQU5TRkVSX0xFTik7DQo+ICsgICAgICAgd3Jp
dGV3KEkyQ19UUkFOQUNfREVGQVVMVF9WQUxVRSwgaTJjLT5iYXNlICsgDQo+ICsgT0ZGU0VUX1RS
QU5TQUNfTEVOKTsNCj4gKw0KPiArICAgICAgIHdyaXRldyhJMkNfVFJBTlNBQ19TVEFSVCB8IEky
Q19SU19NVUxfQ05GRywgaTJjLT5iYXNlICsgDQo+ICsgT0ZGU0VUX1NUQVJUKTsNCj4gKw0KPiAr
ICAgICAgIHJldCA9IHdhaXRfZm9yX2NvbXBsZXRpb25fdGltZW91dCgmaTJjLT5tc2dfY29tcGxl
dGUsDQo+ICsgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGkyYy0+YWRh
cC50aW1lb3V0KTsNCg0KSG93IGRvZXMgdGhlIGhhcmR3YXJlIGtub3cgdGhhdCB0aGlzIHRyYW5z
YWN0aW9uIHNob3VsZCBiZSBhICJtYXN0ZXIgY29kZSI/DQpEbyB5b3UgaGF2ZSB0byB0ZWxsIHRo
ZSBoYXJkd2FyZSB3aGF0IHZhbHVlICgnMDAwMDFYWFgnKSB0byB1c2UgYXMgdGhlIG1hc3RlciBj
b2RlPw0KVGhlIE1hc3RlciBDb2RlIG11c3QgYmUgc2VudCBhdCA8PSA0MDAga0h6LCBub3QgdGhl
IHRhcmdldCBjbG9jay4gIEhvdyBkb2VzIHRoZSBoYXJkd2FyZSBrbm93IHdoYXQgcmF0ZSB0byB1
c2U/DQpXaGVuIHNlbmRpbmcgdGhlIG1hc3RlciBjb2RlLCBhcmJpdHJhdGlvbiBpcyBzdXBwb3Nl
ZCB0byBvY2N1ciwgc3VjaCB0aGF0IG9ubHkgb25lIHdpbm5pbmcgbWFzdGVyIGNhbiBwcm9jZWVk
IHdpdGggdGhlIGZvbGxvd2luZyBoaWdoIHNwZWVkIHRyYW5zYWN0aW9uLg0KV2hlcmUgZG8geW91
IGNoZWNrIHRoYXQgeW91IHdvbiB0aGlzIGFyYml0cmF0aW9uPw0KSWYgdGhpcyBpcyBub3QgaW1w
bGVtZW50ZWQsIGFkZGluZyBhICJUT0RPIiB3b3VsZCBiZSBoZWxwZnVsLg0KDQo+ICsNCj4gKyAg
ICAgICBjb21wbGV0aW9uX2RvbmUoJmkyYy0+bXNnX2NvbXBsZXRlKTsNCg0KVGhpcyBjb21wbGV0
aW9uX2RvbmUoKSBpcyBvbmx5IHVzZWZ1bCBpZiB5b3UgY2hlY2sgdGhlIHJldHVybiB2YWx1ZS4N
CllvdSBzaG91bGQgY2hlY2sgaXQgdG9vLCBzaW5jZSB3ZSBzaG91bGQgb25seSBjaGVjayBmb3Ig
dGltZW91dCBpZiB0aGUgbWVzc2FnZSBoYXNuJ3QgY29tcGxldGVkLg0KDQo+ICsNCj4gKyAgICAg
ICBpZiAocmV0ID09IDApIHsNCj4gKyAgICAgICAgICAgICAgIGRldl9kYmcoaTJjLT5kZXYsICJz
ZW5kIG1hc3RlciBjb2RlIHRpbWVvdXQuXG4iKTsNCj4gKyAgICAgICAgICAgICAgIG10a19pMmNf
aW5pdF9odyhpMmMpOw0KPiArICAgICAgICAgICAgICAgcmV0dXJuIC1FVElNRURPVVQ7DQo+ICsg
ICAgICAgfQ0KPiArDQo+ICsgICAgICAgcmV0dXJuIDA7DQo+ICt9DQo+ICsNCj4gIHN0YXRpYyBp
bnQgbXRrX2kyY19kb190cmFuc2ZlcihzdHJ1Y3QgbXRrX2kyYyAqaTJjLCBzdHJ1Y3QgaTJjX21z
ZyAqbXNncywNCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGludCBudW0sIGludCBs
ZWZ0X251bSkgIHsgQEAgLTUzOSw2IA0KPiArNTc4LDEyIEBAIHN0YXRpYyBpbnQgbXRrX2kyY190
cmFuc2ZlcihzdHJ1Y3QgaTJjX2FkYXB0ZXIgKmFkYXAsDQo+ICAgICAgICAgICAgICAgICB9DQo+
ICAgICAgICAgfQ0KPg0KPiArICAgICAgIGlmIChpMmMtPmF1dG9fcmVzdGFydCAmJiBpMmMtPnNw
ZWVkX2h6ID4gNDAwMDAwKSB7DQoNCkRvbid0IHdlIG5lZWQgdG8gc2VuZCB0aGUgbWFzdGVyIGNv
ZGUgZm9yICpldmVyeSogSFMgdHJhbnNhY3Rpb24sIG5vdCBqdXN0ICJhdXRvX3Jlc3RhcnQiPw0K
DQoiNDAwMDAwIiA9PiBZb3UgYWxyZWFkeSBoYXZlIGEgbWFjcm8gZm9yIHRoaXM6IE1BWF9GU19N
T0RFX1NQRUVEDQoNCj4gKyAgICAgICAgICAgICAgIHJldCA9IG10a19pMmNfc2VuZF9tYXN0ZXJf
Y29kZShpMmMpOw0KPiArICAgICAgICAgICAgICAgaWYgKHJldCkNCj4gKyAgICAgICAgICAgICAg
ICAgICAgICAgcmV0dXJuIHJldDsNCj4gKyAgICAgICB9DQo+ICsNCj4gICAgICAgICB3aGlsZSAo
bGVmdF9udW0tLSkgew0KPiAgICAgICAgICAgICAgICAgaWYgKCFtc2dzLT5idWYpIHsNCj4gICAg
ICAgICAgICAgICAgICAgICAgICAgZGV2X2RiZyhpMmMtPmRldiwgImRhdGEgYnVmZmVyIGlzIE5V
TEwuXG4iKTsNCj4gLS0NCj4gMS44LjEuMS5kaXJ0eQ0KPg0KPiAtLQ0KPiBUbyB1bnN1YnNjcmli
ZSBmcm9tIHRoaXMgbGlzdDogc2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgDQo+IGxpbnV4LWtl
cm5lbCIgaW4gdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2VyLmtlcm5lbC5v
cmcgDQo+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFq
b3Jkb21vLWluZm8uaHRtbA0KPiBQbGVhc2UgcmVhZCB0aGUgRkFRIGF0ICBodHRwOi8vd3d3LnR1
eC5vcmcvbGttbC8NCg0KKioqKioqKioqKioqKiBFbWFpbCBDb25maWRlbnRpYWxpdHkgTm90aWNl
ICoqKioqKioqKioqKioqKioqKioqDQpUaGUgaW5mb3JtYXRpb24gY29udGFpbmVkIGluIHRoaXMg
ZS1tYWlsIG1lc3NhZ2UgKGluY2x1ZGluZyBhbnkgDQphdHRhY2htZW50cykgbWF5IGJlIGNvbmZp
ZGVudGlhbCwgcHJvcHJpZXRhcnksIHByaXZpbGVnZWQsIG9yIG90aGVyd2lzZQ0KZXhlbXB0IGZy
b20gZGlzY2xvc3VyZSB1bmRlciBhcHBsaWNhYmxlIGxhd3MuIEl0IGlzIGludGVuZGVkIHRvIGJl
IA0KY29udmV5ZWQgb25seSB0byB0aGUgZGVzaWduYXRlZCByZWNpcGllbnQocykuIEFueSB1c2Us
IGRpc3NlbWluYXRpb24sIA0KZGlzdHJpYnV0aW9uLCBwcmludGluZywgcmV0YWluaW5nIG9yIGNv
cHlpbmcgb2YgdGhpcyBlLW1haWwgKGluY2x1ZGluZyBpdHMgDQphdHRhY2htZW50cykgYnkgdW5p
bnRlbmRlZCByZWNpcGllbnQocykgaXMgc3RyaWN0bHkgcHJvaGliaXRlZCBhbmQgbWF5IA0KYmUg
dW5sYXdmdWwuIElmIHlvdSBhcmUgbm90IGFuIGludGVuZGVkIHJlY2lwaWVudCBvZiB0aGlzIGUt
bWFpbCwgb3IgYmVsaWV2ZSANCnRoYXQgeW91IGhhdmUgcmVjZWl2ZWQgdGhpcyBlLW1haWwgaW4g
ZXJyb3IsIHBsZWFzZSBub3RpZnkgdGhlIHNlbmRlciANCmltbWVkaWF0ZWx5IChieSByZXBseWlu
ZyB0byB0aGlzIGUtbWFpbCksIGRlbGV0ZSBhbnkgYW5kIGFsbCBjb3BpZXMgb2YgDQp0aGlzIGUt
bWFpbCAoaW5jbHVkaW5nIGFueSBhdHRhY2htZW50cykgZnJvbSB5b3VyIHN5c3RlbSwgYW5kIGRv
IG5vdA0KZGlzY2xvc2UgdGhlIGNvbnRlbnQgb2YgdGhpcyBlLW1haWwgdG8gYW55IG90aGVyIHBl
cnNvbi4gVGhhbmsgeW91IQ0K

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liguo Zhang Dec. 2, 2015, 2:51 a.m. UTC | #4 
On Sat, 2015-11-14 at 22:38 +0800, Daniel Kurtz wrote:
> On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang <liguo.zhang@mediatek.com> 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 <liguo.zhang@mediatek.com>
> > Reviewed-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> >  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 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/


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");