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.
Signed-off-by: Viken Dadhaniya <[email protected]>
---
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);
dma_async_issue_pending(gi2c->tx_c);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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.
> Signed-off-by: Viken Dadhaniya <[email protected]>
You should have a 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
---
bod
> -----Original Message-----
> From: Bryan O'Donoghue <[email protected]>
> Sent: Friday, January 12, 2024 8:25 PM
> To: Viken Dadhaniya (QUIC) <[email protected]>;
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Cc: Mukesh Savaliya (QUIC) <[email protected]>; Visweswara Tanuku
> (QUIC) <[email protected]>
> 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 <[email protected]>
>
> 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