Re: [V3] i2c: i2c-qcom-geni: Correct I2C TRE sequence

[Andi Shyti]

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.

Thanks,
Andi