RE: [PATCH 1/1] i2c: i2c-qcom-geni: Correct I2C TRE sequence

[Viken Dadhaniya]

> -----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