Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751336AbbKNOjI (ORCPT ); Sat, 14 Nov 2015 09:39:08 -0500 Received: from mail-vk0-f43.google.com ([209.85.213.43]:34200 "EHLO mail-vk0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751231AbbKNOjD (ORCPT ); Sat, 14 Nov 2015 09:39:03 -0500 MIME-Version: 1.0 In-Reply-To: <1447047839-5223-3-git-send-email-liguo.zhang@mediatek.com> References: <1447047839-5223-1-git-send-email-liguo.zhang@mediatek.com> <1447047839-5223-3-git-send-email-liguo.zhang@mediatek.com> From: Daniel Kurtz Date: Sat, 14 Nov 2015 22:38:42 +0800 X-Google-Sender-Auth: Mf0lNKLTNA1YjimZPQAkQoGYiO8 Message-ID: Subject: Re: [PATCH v2 2/2] i2c: mediatek: fix i2c multi transfer issue in high speed mode To: Liguo Zhang Cc: Wolfram Sang , srv_heupstream , Matthias Brugger , Eddie Huang , Xudong Chen , Sascha Hauer , Linux I2C , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , linux-mediatek@lists.infradead.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5099 Lines: 133 On Mon, Nov 9, 2015 at 1:43 PM, Liguo Zhang 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 > Reviewed-by: Eddie Huang > --- > 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-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/