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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > Reviewed-by: Eddie Huang <eddie.huang@xxxxxxxxxxxx>
> > ---
> > 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!
Attachment:
signature.asc
Description: Digital signature
