Re: [PATCH v4 2/4] dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively

Mukesh Kumar Savaliya
2024-11-25T05:02:00Z

Hi Konrad, Thanks !

On 11/22/2024 7:10 PM, Konrad Dybcio wrote:
> On 18.11.2024 6:46 AM, Mukesh Kumar Savaliya wrote:
>> Thanks Konrad for the review !
>>
>> On 11/16/2024 12:53 AM, Konrad Dybcio wrote:
>>> On 13.11.2024 5:14 PM, Mukesh Kumar Savaliya wrote:
>>>> GSI DMA provides specific TREs(Transfer ring element) namely Lock and
>>>> Unlock TRE. It provides mutually exclusive access to I2C controller from
>>>> any of the processor(Apps,ADSP). Lock prevents other subsystems from
>>>> concurrently performing DMA transfers and avoids disturbance to data path.
>>>> Basically for shared I2C usecase, lock the SE(Serial Engine) for one of
>>>> the processor, complete the transfer, unlock the SE.
>>>>
>>>> Apply Lock TRE for the first transfer of shared SE and Apply Unlock
>>>> TRE for the last transfer.
>>>>
>>>> Also change MAX_TRE macro to 5 from 3 because of the two additional TREs.
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> ---
>
> [...]
>
>>>>   +    /* create lock tre for first tranfser */
>>>> +    if (i2c->shared_se && i2c->first_msg) {
>>>
>>> Does the first/last logic handle errors well? i.e. what if we
>>> have >= 3 transfers and:
>>>
>>> 1) the first transfer succeeds but the last doesn't
>>> 2) the first transfer succeeds, the second one doesn't and the lock
>>>     is submitted again
>>> 3) the unlock never suceeds
>>>
>> geni_i2c_gpi_xfer() takes care of any of the error. Upon error, it does dma_engine_terminate_sync() which resets all the pipes. Internal downstream also has same implementation.
>
> Okay, this sounds reassuring.
>
> Since the TRE would be locked to APSS, I'm guessing we don't ever need
> to worry about gpi_terminate_all() being executed halfway through a
> non-APSS transaction?
>
Right.
> Konrad