Received-SPF: pass (google.com: domain of linux-kernel+bounces-61583-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <79ce408a-c281-4d22-bc52-1f4cad29d49d@quicinc.com>
Date: Mon, 12 Feb 2024 18:28:36 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [V3] i2c: i2c-qcom-geni: Correct I2C TRE sequence
Content-Language: en-US
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
        Andi Shyti
	<andi.shyti@kernel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
CC: <andersson@kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <linux-i2c@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <vkoul@kernel.org>, <quic_bjorande@quicinc.com>,
        <manivannan.sadhasivam@linaro.org>, <bryan.odonoghue@linaro.org>,
        <quic_msavaliy@quicinc.com>, <quic_vtanuku@quicinc.com>
References: <20240201101323.13676-1-quic_vdadhani@quicinc.com>
 <CAA8EJpqQtHDRK2pex+5F-fMRTosJuFCx59e89MWhnie1O3dHKA@mail.gmail.com>
 <60b5e755-352b-476d-8c6e-2170594ae80d@quicinc.com>
 <uswznu3h53gcefpdc4vxozz32ecdcjvzmr7admwc4h54o27bfy@qqoevrl3dcyt>
 <CAA8EJpqzdp4xYSp+JCExP+Oeu9KhLpsXNUbDxfZ0g+C07xR6dg@mail.gmail.com>
 <cvzyvgb6vahlmrhaijsuyaosdl2p4q5cxhipmu4tujnkpjlbpm@6yu3sbpqha4m>
 <aa7a28a7-59c6-4783-a6de-9a46721a8bf6@linaro.org>
From: Viken Dadhaniya <quic_vdadhani@quicinc.com>
In-Reply-To: <aa7a28a7-59c6-4783-a6de-9a46721a8bf6@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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