| Message ID | 20240201101323.13676-1-quic_vdadhani@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V3] i2c: i2c-qcom-geni: Correct I2C TRE sequence | expand |
On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: > > For i2c read operation in GSI mode, we are getting timeout > due to malformed TRE basically incorrect TRE sequence > in gpi(drivers/dma/qcom/gpi.c) driver. > > TRE stands for Transfer Ring Element - which is basically an element with > size of 4 words. It contains all information like slave address, > clk divider, dma address value data size etc). > > Mainly we have 3 TREs(Config, GO and DMA tre). > - CONFIG TRE : consists of internal register configuration which is > required before start of the transfer. > - DMA TRE : contains DDR/Memory address, called as DMA descriptor. > - GO TRE : contains Transfer directions, slave ID, Delay flags, Length > of the transfer. > > Driver calls GPI driver API to config each TRE depending on the protocol. > If we see GPI driver, for RX operation we are configuring DMA tre and > for TX operation we are configuring GO tre. > > For read operation tre sequence will be as below which is not aligned > to hardware programming guide. > > - CONFIG tre > - DMA tre > - GO tre > > As per Qualcomm's internal Hardware Programming Guide, we should configure > TREs in below sequence for any RX only transfer. > > - CONFIG tre > - GO tre > - DMA tre > > In summary, for RX only transfers, we are reordering DMA and GO TREs. > Tested covering i2c read/write transfer on QCM6490 RB3 board. This hasn't improved. You must describe what is the connection between TRE types and the geni_i2c_gpi calls. It is not obvious until somebody looks into the GPI DMA driver. Another point, for some reason you are still using just the patch version in email subject. Please fix your setup so that the email subject also includes the `[PATCH` part in the subject, which is there by default. Hint: git format-patch -1 -v4 will do that for you without a need to correct anything afterwards. > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> I think you got some review tags for v2, didn't you? They should have been included here, otherwise the efforts spent by the reviewer are lost. > --- > v2 -> v3: > - Update commit log to explain change in simple way. > - Correct fix tag format. > > 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 > >
Hi Viken, On Thu, Feb 01, 2024 at 03:43:23PM +0530, Viken Dadhaniya wrote: > For i2c read operation in GSI mode, we are getting timeout > due to malformed TRE basically incorrect TRE sequence > in gpi(drivers/dma/qcom/gpi.c) driver. > > TRE stands for Transfer Ring Element - which is basically an element with > size of 4 words. It contains all information like slave address, > clk divider, dma address value data size etc). > > Mainly we have 3 TREs(Config, GO and DMA tre). > - CONFIG TRE : consists of internal register configuration which is > required before start of the transfer. > - DMA TRE : contains DDR/Memory address, called as DMA descriptor. > - GO TRE : contains Transfer directions, slave ID, Delay flags, Length > of the transfer. > > Driver calls GPI driver API to config each TRE depending on the protocol. > If we see GPI driver, for RX operation we are configuring DMA tre and > for TX operation we are configuring GO tre. > > For read operation tre sequence will be as below which is not aligned > to hardware programming guide. > > - CONFIG tre > - DMA tre > - GO tre > > As per Qualcomm's internal Hardware Programming Guide, we should configure > TREs in below sequence for any RX only transfer. > > - CONFIG tre > - GO tre > - DMA tre > > In summary, for RX only transfers, we are reordering DMA and GO TREs. > Tested covering i2c read/write transfer on QCM6490 RB3 board. > > Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") > Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> as Dmitry has written, please, next time don't forget the tags: Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5165-rb5 You can also add mine: Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Please make sure to Cc Dmitry who is raising his concerns and check on his comments. Andi PS just as a reminder, if Dmitry's concerns remain related only to the commit log, I gave you the option to agree with him in the e-mail thread without necessarily sending a v4. I can then update the commit log before pushing.
On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: > On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: >> >> For i2c read operation in GSI mode, we are getting timeout >> due to malformed TRE basically incorrect TRE sequence >> in gpi(drivers/dma/qcom/gpi.c) driver. >> >> TRE stands for Transfer Ring Element - which is basically an element with >> size of 4 words. It contains all information like slave address, >> clk divider, dma address value data size etc). >> >> Mainly we have 3 TREs(Config, GO and DMA tre). >> - CONFIG TRE : consists of internal register configuration which is >> required before start of the transfer. >> - DMA TRE : contains DDR/Memory address, called as DMA descriptor. >> - GO TRE : contains Transfer directions, slave ID, Delay flags, Length >> of the transfer. >> >> Driver calls GPI driver API to config each TRE depending on the protocol. >> If we see GPI driver, for RX operation we are configuring DMA tre and >> for TX operation we are configuring GO tre. >> >> For read operation tre sequence will be as below which is not aligned >> to hardware programming guide. >> >> - CONFIG tre >> - DMA tre >> - GO tre >> >> As per Qualcomm's internal Hardware Programming Guide, we should configure >> TREs in below sequence for any RX only transfer. >> >> - CONFIG tre >> - GO tre >> - DMA tre >> >> In summary, for RX only transfers, we are reordering DMA and GO TREs. >> Tested covering i2c read/write transfer on QCM6490 RB3 board. > > This hasn't improved. You must describe what is the connection between > TRE types and the geni_i2c_gpi calls. > It is not obvious until somebody looks into the GPI DMA driver. > > Another point, for some reason you are still using just the patch > version in email subject. Please fix your setup so that the email > subject also includes the `[PATCH` part in the subject, which is there > by default. > Hint: git format-patch -1 -v4 will do that for you without a need to > correct anything afterwards. > At high level, let me explain the I2C to GPI driver flow in general. I2C driver calls GPI driver exposed functions which will prepare all the TREs as per programming guide and queues to the GPI DMA engine for execution. Upon completion of the Transfer, GPI DMA engine will generate an interrupt which will be handled inside the GPIO driver. Then GPI driver will call DMA framework registered callback by i2c. Upon receiving this callback, i2c driver marks the transfer completion. >> >> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") >> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> > > I think you got some review tags for v2, didn't you? They should have > been included here, otherwise the efforts spent by the reviewer are > lost. > Sorry, missed to add reviewed-by tag. Andi will help to update. >> --- >> v2 -> v3: >> - Update commit log to explain change in simple way. >> - Correct fix tag format. >> >> 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 >> >> > >
Hi Andi,
Thanks for review and taking care of patch.
We have responded to dmitry to describe more about GPI operation and
tried to explain flow in general.
We shall keep updating over email for commit log.
Please help amend below tags in commit log since we are taking it over
email:
Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5165-rb5
Co-developed-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
On 2/1/2024 7:52 PM, Andi Shyti wrote:
> Hi Viken,
>
> On Thu, Feb 01, 2024 at 03:43:23PM +0530, Viken Dadhaniya wrote:
>> For i2c read operation in GSI mode, we are getting timeout
>> due to malformed TRE basically incorrect TRE sequence
>> in gpi(drivers/dma/qcom/gpi.c) driver.
>>
>> TRE stands for Transfer Ring Element - which is basically an element with
>> size of 4 words. It contains all information like slave address,
>> clk divider, dma address value data size etc).
>>
>> Mainly we have 3 TREs(Config, GO and DMA tre).
>> - CONFIG TRE : consists of internal register configuration which is
>> required before start of the transfer.
>> - DMA TRE : contains DDR/Memory address, called as DMA descriptor.
>> - GO TRE : contains Transfer directions, slave ID, Delay flags, Length
>> of the transfer.
>>
>> Driver calls GPI driver API to config each TRE depending on the protocol.
>> If we see GPI driver, for RX operation we are configuring DMA tre and
>> for TX operation we are configuring GO tre.
>>
>> For read operation tre sequence will be as below which is not aligned
>> to hardware programming guide.
>>
>> - CONFIG tre
>> - DMA tre
>> - GO tre
>>
>> As per Qualcomm's internal Hardware Programming Guide, we should configure
>> TREs in below sequence for any RX only transfer.
>>
>> - CONFIG tre
>> - GO tre
>> - DMA tre
>>
>> In summary, for RX only transfers, we are reordering DMA and GO TREs.
>> Tested covering i2c read/write transfer on QCM6490 RB3 board.
>>
>> Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA")
>> Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com>
>
> as Dmitry has written, please, next time don't forget the tags:
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5165-rb5
>
> You can also add mine:
>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
>
> Please make sure to Cc Dmitry who is raising his concerns and
> check on his comments.
>
> Andi
>
> PS just as a reminder, if Dmitry's concerns remain related only
> to the commit log, I gave you the option to agree with him in the
> e-mail thread without necessarily sending a v4. I can then update
> the commit log before pushing.
Hi Viken, Dmitry, On Fri, Feb 02, 2024 at 04:13:06PM +0530, Viken Dadhaniya wrote: > > On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: > > On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: > > > > > > For i2c read operation in GSI mode, we are getting timeout > > > due to malformed TRE basically incorrect TRE sequence > > > in gpi(drivers/dma/qcom/gpi.c) driver. > > > > > > TRE stands for Transfer Ring Element - which is basically an element with > > > size of 4 words. It contains all information like slave address, > > > clk divider, dma address value data size etc). > > > > > > Mainly we have 3 TREs(Config, GO and DMA tre). > > > - CONFIG TRE : consists of internal register configuration which is > > > required before start of the transfer. > > > - DMA TRE : contains DDR/Memory address, called as DMA descriptor. > > > - GO TRE : contains Transfer directions, slave ID, Delay flags, Length > > > of the transfer. > > > > > > Driver calls GPI driver API to config each TRE depending on the protocol. > > > If we see GPI driver, for RX operation we are configuring DMA tre and > > > for TX operation we are configuring GO tre. > > > > > > For read operation tre sequence will be as below which is not aligned > > > to hardware programming guide. > > > > > > - CONFIG tre > > > - DMA tre > > > - GO tre > > > > > > As per Qualcomm's internal Hardware Programming Guide, we should configure > > > TREs in below sequence for any RX only transfer. > > > > > > - CONFIG tre > > > - GO tre > > > - DMA tre > > > > > > In summary, for RX only transfers, we are reordering DMA and GO TREs. > > > Tested covering i2c read/write transfer on QCM6490 RB3 board. > > > > This hasn't improved. You must describe what is the connection between > > TRE types and the geni_i2c_gpi calls. > > It is not obvious until somebody looks into the GPI DMA driver. > > > > Another point, for some reason you are still using just the patch > > version in email subject. Please fix your setup so that the email > > subject also includes the `[PATCH` part in the subject, which is there > > by default. > > Hint: git format-patch -1 -v4 will do that for you without a need to > > correct anything afterwards. > > > > At high level, let me explain the I2C to GPI driver flow in general. > > I2C driver calls GPI driver exposed functions which will prepare all the > TREs as per programming guide and > queues to the GPI DMA engine for execution. Upon completion of the Transfer, > GPI DMA engine will generate an > interrupt which will be handled inside the GPIO driver. Then GPI driver will > call DMA framework registered callback by i2c. > Upon receiving this callback, i2c driver marks the transfer completion. Any news about this? Dmitry do you still have concerns? We can add this last description in the commit log, as well, if needed. Andi
On Thu, 8 Feb 2024 at 12:02, Andi Shyti <andi.shyti@kernel.org> wrote: > > Hi Viken, Dmitry, > > On Fri, Feb 02, 2024 at 04:13:06PM +0530, Viken Dadhaniya wrote: > > > > On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: > > > On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: > > > > > > > > For i2c read operation in GSI mode, we are getting timeout > > > > due to malformed TRE basically incorrect TRE sequence > > > > in gpi(drivers/dma/qcom/gpi.c) driver. > > > > > > > > TRE stands for Transfer Ring Element - which is basically an element with > > > > size of 4 words. It contains all information like slave address, > > > > clk divider, dma address value data size etc). > > > > > > > > Mainly we have 3 TREs(Config, GO and DMA tre). > > > > - CONFIG TRE : consists of internal register configuration which is > > > > required before start of the transfer. > > > > - DMA TRE : contains DDR/Memory address, called as DMA descriptor. > > > > - GO TRE : contains Transfer directions, slave ID, Delay flags, Length > > > > of the transfer. > > > > > > > > Driver calls GPI driver API to config each TRE depending on the protocol. > > > > If we see GPI driver, for RX operation we are configuring DMA tre and > > > > for TX operation we are configuring GO tre. > > > > > > > > For read operation tre sequence will be as below which is not aligned > > > > to hardware programming guide. > > > > > > > > - CONFIG tre > > > > - DMA tre > > > > - GO tre > > > > > > > > As per Qualcomm's internal Hardware Programming Guide, we should configure > > > > TREs in below sequence for any RX only transfer. > > > > > > > > - CONFIG tre > > > > - GO tre > > > > - DMA tre > > > > > > > > In summary, for RX only transfers, we are reordering DMA and GO TREs. > > > > Tested covering i2c read/write transfer on QCM6490 RB3 board. > > > > > > This hasn't improved. You must describe what is the connection between > > > TRE types and the geni_i2c_gpi calls. > > > It is not obvious until somebody looks into the GPI DMA driver. > > > > > > Another point, for some reason you are still using just the patch > > > version in email subject. Please fix your setup so that the email > > > subject also includes the `[PATCH` part in the subject, which is there > > > by default. > > > Hint: git format-patch -1 -v4 will do that for you without a need to > > > correct anything afterwards. > > > > > > > At high level, let me explain the I2C to GPI driver flow in general. > > > > I2C driver calls GPI driver exposed functions which will prepare all the > > TREs as per programming guide and > > queues to the GPI DMA engine for execution. Upon completion of the Transfer, > > GPI DMA engine will generate an > > interrupt which will be handled inside the GPIO driver. Then GPI driver will > > call DMA framework registered callback by i2c. > > Upon receiving this callback, i2c driver marks the transfer completion. > > Any news about this? Dmitry do you still have concerns? We can > add this last description in the commit log, as well, if needed. I was looking for pretty simple addition to the commit message, that links existing commit message to the actual source code change: that geni_i2c_gpi(I2C_WRITE) results in the GO TRE and geni_i2c_gpi(I2C_READ) generates DMA TRE. But I haven't seen anything sensible up to now. So far we have a nice description of required programming sequence in terms of CONFIG, GO, DMA TREs and then source code change that seems completely unrelated to the commit message, unless one actually goes deep into the corresponding GPI DMA driver.
Hi Dmitry, On Thu, Feb 08, 2024 at 01:04:14PM +0200, Dmitry Baryshkov wrote: > On Thu, 8 Feb 2024 at 12:02, Andi Shyti <andi.shyti@kernel.org> wrote: > > > > Hi Viken, Dmitry, > > > > On Fri, Feb 02, 2024 at 04:13:06PM +0530, Viken Dadhaniya wrote: > > > > > > On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: > > > > On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: > > > > > > > > > > For i2c read operation in GSI mode, we are getting timeout > > > > > due to malformed TRE basically incorrect TRE sequence > > > > > in gpi(drivers/dma/qcom/gpi.c) driver. > > > > > > > > > > TRE stands for Transfer Ring Element - which is basically an element with > > > > > size of 4 words. It contains all information like slave address, > > > > > clk divider, dma address value data size etc). > > > > > > > > > > Mainly we have 3 TREs(Config, GO and DMA tre). > > > > > - CONFIG TRE : consists of internal register configuration which is > > > > > required before start of the transfer. > > > > > - DMA TRE : contains DDR/Memory address, called as DMA descriptor. > > > > > - GO TRE : contains Transfer directions, slave ID, Delay flags, Length > > > > > of the transfer. > > > > > > > > > > Driver calls GPI driver API to config each TRE depending on the protocol. > > > > > If we see GPI driver, for RX operation we are configuring DMA tre and > > > > > for TX operation we are configuring GO tre. > > > > > > > > > > For read operation tre sequence will be as below which is not aligned > > > > > to hardware programming guide. > > > > > > > > > > - CONFIG tre > > > > > - DMA tre > > > > > - GO tre > > > > > > > > > > As per Qualcomm's internal Hardware Programming Guide, we should configure > > > > > TREs in below sequence for any RX only transfer. > > > > > > > > > > - CONFIG tre > > > > > - GO tre > > > > > - DMA tre > > > > > > > > > > In summary, for RX only transfers, we are reordering DMA and GO TREs. > > > > > Tested covering i2c read/write transfer on QCM6490 RB3 board. > > > > > > > > This hasn't improved. You must describe what is the connection between > > > > TRE types and the geni_i2c_gpi calls. > > > > It is not obvious until somebody looks into the GPI DMA driver. > > > > > > > > Another point, for some reason you are still using just the patch > > > > version in email subject. Please fix your setup so that the email > > > > subject also includes the `[PATCH` part in the subject, which is there > > > > by default. > > > > Hint: git format-patch -1 -v4 will do that for you without a need to > > > > correct anything afterwards. > > > > > > > > > > At high level, let me explain the I2C to GPI driver flow in general. > > > > > > I2C driver calls GPI driver exposed functions which will prepare all the > > > TREs as per programming guide and > > > queues to the GPI DMA engine for execution. Upon completion of the Transfer, > > > GPI DMA engine will generate an > > > interrupt which will be handled inside the GPIO driver. Then GPI driver will > > > call DMA framework registered callback by i2c. > > > Upon receiving this callback, i2c driver marks the transfer completion. > > > > Any news about this? Dmitry do you still have concerns? We can > > add this last description in the commit log, as well, if needed. > > I was looking for pretty simple addition to the commit message, that > links existing commit message to the actual source code change: that > geni_i2c_gpi(I2C_WRITE) results in the GO TRE and > geni_i2c_gpi(I2C_READ) generates DMA TRE. But I haven't seen anything > sensible up to now. So far we have a nice description of required > programming sequence in terms of CONFIG, GO, DMA TREs and then source > code change that seems completely unrelated to the commit message, > unless one actually goes deep into the corresponding GPI DMA driver. Agree. I can't take this patch until the commit message has a proper description and until Dmitry doesn't have any concerns pending. Thanks, Andi
On 8.02.2024 12:59, Andi Shyti wrote: > Hi Dmitry, > > On Thu, Feb 08, 2024 at 01:04:14PM +0200, Dmitry Baryshkov wrote: >> On Thu, 8 Feb 2024 at 12:02, Andi Shyti <andi.shyti@kernel.org> wrote: >>> >>> Hi Viken, Dmitry, >>> >>> On Fri, Feb 02, 2024 at 04:13:06PM +0530, Viken Dadhaniya wrote: >>>> >>>> On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: >>>>> On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: >>>>>> >>>>>> For i2c read operation in GSI mode, we are getting timeout >>>>>> due to malformed TRE basically incorrect TRE sequence >>>>>> in gpi(drivers/dma/qcom/gpi.c) driver. >>>>>> >>>>>> TRE stands for Transfer Ring Element - which is basically an element with >>>>>> size of 4 words. It contains all information like slave address, >>>>>> clk divider, dma address value data size etc). >>>>>> >>>>>> Mainly we have 3 TREs(Config, GO and DMA tre). >>>>>> - CONFIG TRE : consists of internal register configuration which is >>>>>> required before start of the transfer. >>>>>> - DMA TRE : contains DDR/Memory address, called as DMA descriptor. >>>>>> - GO TRE : contains Transfer directions, slave ID, Delay flags, Length >>>>>> of the transfer. >>>>>> >>>>>> Driver calls GPI driver API to config each TRE depending on the protocol. >>>>>> If we see GPI driver, for RX operation we are configuring DMA tre and >>>>>> for TX operation we are configuring GO tre. >>>>>> >>>>>> For read operation tre sequence will be as below which is not aligned >>>>>> to hardware programming guide. >>>>>> >>>>>> - CONFIG tre >>>>>> - DMA tre >>>>>> - GO tre >>>>>> >>>>>> As per Qualcomm's internal Hardware Programming Guide, we should configure >>>>>> TREs in below sequence for any RX only transfer. >>>>>> >>>>>> - CONFIG tre >>>>>> - GO tre >>>>>> - DMA tre >>>>>> >>>>>> In summary, for RX only transfers, we are reordering DMA and GO TREs. >>>>>> Tested covering i2c read/write transfer on QCM6490 RB3 board. >>>>> >>>>> This hasn't improved. You must describe what is the connection between >>>>> TRE types and the geni_i2c_gpi calls. >>>>> It is not obvious until somebody looks into the GPI DMA driver. >>>>> >>>>> Another point, for some reason you are still using just the patch >>>>> version in email subject. Please fix your setup so that the email >>>>> subject also includes the `[PATCH` part in the subject, which is there >>>>> by default. >>>>> Hint: git format-patch -1 -v4 will do that for you without a need to >>>>> correct anything afterwards. >>>>> >>>> >>>> At high level, let me explain the I2C to GPI driver flow in general. >>>> >>>> I2C driver calls GPI driver exposed functions which will prepare all the >>>> TREs as per programming guide and >>>> queues to the GPI DMA engine for execution. Upon completion of the Transfer, >>>> GPI DMA engine will generate an >>>> interrupt which will be handled inside the GPIO driver. Then GPI driver will >>>> call DMA framework registered callback by i2c. >>>> Upon receiving this callback, i2c driver marks the transfer completion. >>> >>> Any news about this? Dmitry do you still have concerns? We can >>> add this last description in the commit log, as well, if needed. >> >> I was looking for pretty simple addition to the commit message, that >> links existing commit message to the actual source code change: that >> geni_i2c_gpi(I2C_WRITE) results in the GO TRE and >> geni_i2c_gpi(I2C_READ) generates DMA TRE. But I haven't seen anything >> sensible up to now. So far we have a nice description of required >> programming sequence in terms of CONFIG, GO, DMA TREs and then source >> code change that seems completely unrelated to the commit message, >> unless one actually goes deep into the corresponding GPI DMA driver. > > Agree. I can't take this patch until the commit message has a > proper description and until Dmitry doesn't have any concerns > pending. And please, please, include the word PATCH in the square brackets in the subject, it's landing in the wrong email folders for a number of folks.. Konrad
On 2/8/2024 4:34 PM, Dmitry Baryshkov wrote: > On Thu, 8 Feb 2024 at 12:02, Andi Shyti <andi.shyti@kernel.org> wrote: >> >> Hi Viken, Dmitry, >> >> On Fri, Feb 02, 2024 at 04:13:06PM +0530, Viken Dadhaniya wrote: >>> >>> On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: >>>> On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: >>>>> >>>>> For i2c read operation in GSI mode, we are getting timeout >>>>> due to malformed TRE basically incorrect TRE sequence >>>>> in gpi(drivers/dma/qcom/gpi.c) driver. >>>>> >>>>> TRE stands for Transfer Ring Element - which is basically an element with >>>>> size of 4 words. It contains all information like slave address, >>>>> clk divider, dma address value data size etc). >>>>> >>>>> Mainly we have 3 TREs(Config, GO and DMA tre). >>>>> - CONFIG TRE : consists of internal register configuration which is >>>>> required before start of the transfer. >>>>> - DMA TRE : contains DDR/Memory address, called as DMA descriptor. >>>>> - GO TRE : contains Transfer directions, slave ID, Delay flags, Length >>>>> of the transfer. >>>>> >>>>> Driver calls GPI driver API to config each TRE depending on the protocol. >>>>> If we see GPI driver, for RX operation we are configuring DMA tre and >>>>> for TX operation we are configuring GO tre. >>>>> >>>>> For read operation tre sequence will be as below which is not aligned >>>>> to hardware programming guide. >>>>> >>>>> - CONFIG tre >>>>> - DMA tre >>>>> - GO tre >>>>> >>>>> As per Qualcomm's internal Hardware Programming Guide, we should configure >>>>> TREs in below sequence for any RX only transfer. >>>>> >>>>> - CONFIG tre >>>>> - GO tre >>>>> - DMA tre >>>>> >>>>> In summary, for RX only transfers, we are reordering DMA and GO TREs. >>>>> Tested covering i2c read/write transfer on QCM6490 RB3 board. >>>> >>>> This hasn't improved. You must describe what is the connection between >>>> TRE types and the geni_i2c_gpi calls. >>>> It is not obvious until somebody looks into the GPI DMA driver. >>>> >>>> Another point, for some reason you are still using just the patch >>>> version in email subject. Please fix your setup so that the email >>>> subject also includes the `[PATCH` part in the subject, which is there >>>> by default. >>>> Hint: git format-patch -1 -v4 will do that for you without a need to >>>> correct anything afterwards. >>>> >>> >>> At high level, let me explain the I2C to GPI driver flow in general. >>> >>> I2C driver calls GPI driver exposed functions which will prepare all the >>> TREs as per programming guide and >>> queues to the GPI DMA engine for execution. Upon completion of the Transfer, >>> GPI DMA engine will generate an >>> interrupt which will be handled inside the GPIO driver. Then GPI driver will >>> call DMA framework registered callback by i2c. >>> Upon receiving this callback, i2c driver marks the transfer completion. >> >> Any news about this? Dmitry do you still have concerns? We can >> add this last description in the commit log, as well, if needed. > > I was looking for pretty simple addition to the commit message, that > links existing commit message to the actual source code change: that > geni_i2c_gpi(I2C_WRITE) results in the GO TRE and > geni_i2c_gpi(I2C_READ) generates DMA TRE. But I haven't seen anything > sensible up to now. So far we have a nice description of required > programming sequence in terms of CONFIG, GO, DMA TREs and then source > code change that seems completely unrelated to the commit message, > unless one actually goes deep into the corresponding GPI DMA driver. > Updated commit log in V4
On 2/9/2024 8:11 PM, Konrad Dybcio wrote: > On 8.02.2024 12:59, Andi Shyti wrote: >> Hi Dmitry, >> >> On Thu, Feb 08, 2024 at 01:04:14PM +0200, Dmitry Baryshkov wrote: >>> On Thu, 8 Feb 2024 at 12:02, Andi Shyti <andi.shyti@kernel.org> wrote: >>>> >>>> Hi Viken, Dmitry, >>>> >>>> On Fri, Feb 02, 2024 at 04:13:06PM +0530, Viken Dadhaniya wrote: >>>>> >>>>> On 2/1/2024 5:24 PM, Dmitry Baryshkov wrote: >>>>>> On Thu, 1 Feb 2024 at 12:13, Viken Dadhaniya <quic_vdadhani@quicinc.com> wrote: >>>>>>> >>>>>>> For i2c read operation in GSI mode, we are getting timeout >>>>>>> due to malformed TRE basically incorrect TRE sequence >>>>>>> in gpi(drivers/dma/qcom/gpi.c) driver. >>>>>>> >>>>>>> TRE stands for Transfer Ring Element - which is basically an element with >>>>>>> size of 4 words. It contains all information like slave address, >>>>>>> clk divider, dma address value data size etc). >>>>>>> >>>>>>> Mainly we have 3 TREs(Config, GO and DMA tre). >>>>>>> - CONFIG TRE : consists of internal register configuration which is >>>>>>> required before start of the transfer. >>>>>>> - DMA TRE : contains DDR/Memory address, called as DMA descriptor. >>>>>>> - GO TRE : contains Transfer directions, slave ID, Delay flags, Length >>>>>>> of the transfer. >>>>>>> >>>>>>> Driver calls GPI driver API to config each TRE depending on the protocol. >>>>>>> If we see GPI driver, for RX operation we are configuring DMA tre and >>>>>>> for TX operation we are configuring GO tre. >>>>>>> >>>>>>> For read operation tre sequence will be as below which is not aligned >>>>>>> to hardware programming guide. >>>>>>> >>>>>>> - CONFIG tre >>>>>>> - DMA tre >>>>>>> - GO tre >>>>>>> >>>>>>> As per Qualcomm's internal Hardware Programming Guide, we should configure >>>>>>> TREs in below sequence for any RX only transfer. >>>>>>> >>>>>>> - CONFIG tre >>>>>>> - GO tre >>>>>>> - DMA tre >>>>>>> >>>>>>> In summary, for RX only transfers, we are reordering DMA and GO TREs. >>>>>>> Tested covering i2c read/write transfer on QCM6490 RB3 board. >>>>>> >>>>>> This hasn't improved. You must describe what is the connection between >>>>>> TRE types and the geni_i2c_gpi calls. >>>>>> It is not obvious until somebody looks into the GPI DMA driver. >>>>>> >>>>>> Another point, for some reason you are still using just the patch >>>>>> version in email subject. Please fix your setup so that the email >>>>>> subject also includes the `[PATCH` part in the subject, which is there >>>>>> by default. >>>>>> Hint: git format-patch -1 -v4 will do that for you without a need to >>>>>> correct anything afterwards. >>>>>> >>>>> >>>>> At high level, let me explain the I2C to GPI driver flow in general. >>>>> >>>>> I2C driver calls GPI driver exposed functions which will prepare all the >>>>> TREs as per programming guide and >>>>> queues to the GPI DMA engine for execution. Upon completion of the Transfer, >>>>> GPI DMA engine will generate an >>>>> interrupt which will be handled inside the GPIO driver. Then GPI driver will >>>>> call DMA framework registered callback by i2c. >>>>> Upon receiving this callback, i2c driver marks the transfer completion. >>>> >>>> Any news about this? Dmitry do you still have concerns? We can >>>> add this last description in the commit log, as well, if needed. >>> >>> I was looking for pretty simple addition to the commit message, that >>> links existing commit message to the actual source code change: that >>> geni_i2c_gpi(I2C_WRITE) results in the GO TRE and >>> geni_i2c_gpi(I2C_READ) generates DMA TRE. But I haven't seen anything >>> sensible up to now. So far we have a nice description of required >>> programming sequence in terms of CONFIG, GO, DMA TREs and then source >>> code change that seems completely unrelated to the commit message, >>> unless one actually goes deep into the corresponding GPI DMA driver. >> >> Agree. I can't take this patch until the commit message has a >> proper description and until Dmitry doesn't have any concerns >> pending. > > And please, please, include the word PATCH in the square brackets in > the subject, it's landing in the wrong email folders for a number of > folks.. Included "PATCH" string in subject in V4. > > Konrad
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 in GSI mode, we are getting timeout due to malformed TRE basically incorrect TRE sequence in gpi(drivers/dma/qcom/gpi.c) driver. TRE stands for Transfer Ring Element - which is basically an element with size of 4 words. It contains all information like slave address, clk divider, dma address value data size etc). Mainly we have 3 TREs(Config, GO and DMA tre). - CONFIG TRE : consists of internal register configuration which is required before start of the transfer. - DMA TRE : contains DDR/Memory address, called as DMA descriptor. - GO TRE : contains Transfer directions, slave ID, Delay flags, Length of the transfer. Driver calls GPI driver API to config each TRE depending on the protocol. If we see GPI driver, for RX operation we are configuring DMA tre and for TX operation we are configuring GO tre. For read operation tre sequence will be as below which is not aligned to hardware programming guide. - CONFIG tre - DMA tre - GO tre As per Qualcomm's internal Hardware Programming Guide, we should configure TREs in below sequence for any RX only transfer. - CONFIG tre - GO tre - DMA tre In summary, for RX only transfers, we are reordering DMA and GO TREs. Tested covering i2c read/write transfer on QCM6490 RB3 board. Fixes: d8703554f4de ("i2c: qcom-geni: Add support for GPI DMA") Signed-off-by: Viken Dadhaniya <quic_vdadhani@quicinc.com> --- v2 -> v3: - Update commit log to explain change in simple way. - Correct fix tag format. 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(-)