> -----Original Message-----
> From: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> Sent: Friday, January 12, 2024 8:25 PM
> To: Viken Dadhaniya (QUIC) <quic_vdadhani@xxxxxxxxxxx>;
> andersson@xxxxxxxxxx; konrad.dybcio@xxxxxxxxxx; andi.shyti@xxxxxxxxxx; linux-
> arm-msm@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; vkoul@xxxxxxxxxx
> Cc: Mukesh Savaliya (QUIC) <quic_msavaliy@xxxxxxxxxxx>; Visweswara Tanuku
> (QUIC) <quic_vtanuku@xxxxxxxxxxx>
> Subject: Re: [PATCH 1/1] i2c: i2c-qcom-geni: Correct I2C TRE sequence
>
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> On 12/01/2024 13:53, Viken Dadhaniya wrote:
> > For i2c read operation, we are getting gsi mode timeout due to
> > malformed TRE(Transfer Ring Element). currently for read opreration,
> > we are configuring incorrect TRE sequence(config->dma->go).
> >
> > So correct TRE sequence(config->go->dma) to resolve timeout issue for
> > read operation.
>
> I don't think this commit log really captures what the code does.
>
> - Sets up optional RX DMA
> - Sets up TX DMA
> - Issues optional RX dma_async_issue_pending
> - Issues TX dma_async_issue_pending
>
> What your change does is sets up the TX DMA first
>
> - Sets up TX DMA
> - Sets up optional RX DMA
> - Issues optional RX dma_async_issue_pending
> - Issues TX dma_async_issue_pending
>
> but you've not really root-caused by re-ordering the calls fixes anything for you.
>
> This may be the right fix but I don't really think you've captured here in the
> commit log _why_ its the right fix if indeed it is correct.
Updated commit massage with proper information.
>
> > Signed-off-by: Viken Dadhaniya <quic_vdadhani@xxxxxxxxxxx>
>
> You should have a Fixes: tag
Added fixes tag.
>
> > ---
> > drivers/i2c/busses/i2c-qcom-geni.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
> > b/drivers/i2c/busses/i2c-qcom-geni.c
> > index 0d2e7171e3a6..5904fc8bba71 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -613,6 +613,11 @@ 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); @@ -620,11 +625,6 @@ static int geni_i2c_gpi_xfer(struct
> geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
> > 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);
>
> If TX gets moved up top then the second check for if (msgs[i].flags &
> I2C_M_RD) is redundant.
>
> You could just have
>
> 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;
>
> dma_async_issue_pending(gi2c->rx_c);
> }
>
> - Please investigate further.
> Why/how does the new sequence
>
> TX DMA setup
> RX DMA setup
> RX DMA sync
> TX DMA sync
>
> Improve the situation over the existing and more logical
>
> RX DMA setup
> TX DMA setup
> RX DMA sync
> TX DMA sync
>
> - Add a Fixes tag if you work that out so we know
> which kernel version to back port to
>
> - Include the SoC version(s) you have tested on in the commit
> or cover letter
>
> - And drop the redundant check
Removed redundant check.
Added SoC information.
>
> ---
> bod
