| Message ID | 20240129061003.4085-1-quic_vdadhani@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] i2c: i2c-qcom-geni: Correct I2C TRE sequence | expand |
On 1/30/2024 5:19 AM, Dmitry Baryshkov wrote: > On Mon, 29 Jan 2024 at 08:10, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: >> >> For i2c read operation, we are getting gsi mode timeout due >> to malformed TRE(Transfer Ring Element). Currently we are >> configuring incorrect TRE sequence in gpi driver >> (drivers/dma/qcom/gpi.c) as below >> >> - Sets up CONFIG >> - Sets up DMA tre >> - Sets up GO tre >> >> As per HPG(Hardware programming guide), We should configure TREs in below >> sequence for any i2c transfer >> >> - Sets up CONFIG tre >> - Sets up GO tre >> - Sets up DMA tre > > It is not clear how this is relevant and/or affected by swapping > I2C_WRITE and I2C_READ gpi calls. > Submitted V3 with proper commit log. >> >> For only write operation or write followed by read operation, >> existing software sequence is correct. >> >> for only read operation, TRE sequence need to be corrected. >> Hence, we have changed the sequence to submit GO tre before DMA tre. >> >> Tested covering i2c read/write transfer on QCM6490 RB3 board. > > Please read Documentation/process/submitting-patches.rst, understand > it and write a proper commit message. > >> >> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> >> Fixes: commit d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > > As it was pointed out, this line shows ignorance of the mentioned file > and of the existing community practices. > Updated fixes tag in V3. >> --- >> v1 -> v2: >> - Remove redundant check. >> - update commit log. >> - add fix tag. >> --- >> --- >> drivers/i2c/busses/i2c-qcom-geni.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c >> index 0d2e7171e3a6..da94df466e83 100644 >> --- a/drivers/i2c/busses/i2c-qcom-geni.c >> +++ b/drivers/i2c/busses/i2c-qcom-geni.c >> @@ -613,20 +613,20 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i >> >> peripheral.addr = msgs[i].addr; >> >> + ret = geni_i2c_gpi(gi2c, &msgs[i], &config, >> + &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c); >> + if (ret) >> + goto err; >> + >> if (msgs[i].flags & I2C_M_RD) { >> ret = geni_i2c_gpi(gi2c, &msgs[i], &config, >> &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c); >> if (ret) >> goto err; >> - } >> - >> - ret = geni_i2c_gpi(gi2c, &msgs[i], &config, >> - &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c); >> - if (ret) >> - goto err; >> >> - if (msgs[i].flags & I2C_M_RD) >> dma_async_issue_pending(gi2c->rx_c); >> + } >> + >> dma_async_issue_pending(gi2c->tx_c); >> >> timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT); >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation >> >> > >
On 1/30/2024 8:54 PM, Bryan O'Donoghue wrote: > On 29/01/2024 06:10, Viken Dadhaniya wrote: >> As per HPG(Hardware programming guide > > Since you are doing a V3 here, please amend this too. > > "As per HPG" means almost nothing outside of qcom. > > "As per Qualcomm's internal Hardware Programming Guide" > > Right thing to do to reference the document IMO but, you should make > clear its an internal silicon specification that's not public. > > Also not sure the TRE description adds much. > > Just give a high level description of the sequences being out-of-order > with respect to the hardware spec. > > --- > bod Updated commit log and provided TRE description with sequence in V3.
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index 0d2e7171e3a6..da94df466e83 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -613,20 +613,20 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i peripheral.addr = msgs[i].addr; + ret = geni_i2c_gpi(gi2c, &msgs[i], &config, + &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c); + if (ret) + goto err; + if (msgs[i].flags & I2C_M_RD) { ret = geni_i2c_gpi(gi2c, &msgs[i], &config, &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c); if (ret) goto err; - } - - ret = geni_i2c_gpi(gi2c, &msgs[i], &config, - &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c); - if (ret) - goto err; - if (msgs[i].flags & I2C_M_RD) dma_async_issue_pending(gi2c->rx_c); + } + dma_async_issue_pending(gi2c->tx_c); timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
For i2c read operation, we are getting gsi mode timeout due to malformed TRE(Transfer Ring Element). Currently we are configuring incorrect TRE sequence in gpi driver (drivers/dma/qcom/gpi.c) as below - Sets up CONFIG - Sets up DMA tre - Sets up GO tre As per HPG(Hardware programming guide), We should configure TREs in below sequence for any i2c transfer - Sets up CONFIG tre - Sets up GO tre - Sets up DMA tre For only write operation or write followed by read operation, existing software sequence is correct. for only read operation, TRE sequence need to be corrected. Hence, we have changed the sequence to submit GO tre before DMA tre. Tested covering i2c read/write transfer on QCM6490 RB3 board. Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> Fixes: commit d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") --- v1 -> v2: - Remove redundant check. - update commit log. - add fix tag. --- --- drivers/i2c/busses/i2c-qcom-geni.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)