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@xxxxxxxxxx> 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@xxxxxxxxxxx> 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
