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

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

On 12/24/2024 3:28 PM, Vinod Koul wrote:
> On 18-12-24, 18:04, Mukesh Kumar Savaliya wrote:
>> Hi Vinod, Thanks !  I just saw your comments now as somehow it was going in
>> some other folder and didn't realize.
>>
>> On 12/4/2024 5:51 PM, Vinod Koul wrote:
>>> On 02-12-24, 16:13, Mukesh Kumar Savaliya wrote:
>>>> Thanks for the review comments Vinod !
>>>>
>>>> On 12/2/2024 12:17 PM, Vinod Koul wrote:
>>>>> On 29-11-24, 20:13, 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.
>>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> @@ -65,6 +65,9 @@ enum i2c_op {
>>>>>>      * @rx_len: receive length for buffer
>>>>>>      * @op: i2c cmd
>>>>>>      * @muli-msg: is part of multi i2c r-w msgs
>>>>>> + * @shared_se: bus is shared between subsystems
>>>>>> + * @bool first_msg: use it for tracking multimessage xfer
>>>>>> + * @bool last_msg: use it for tracking multimessage xfer
>>>>>>      */
>>>>>>     struct gpi_i2c_config {
>>>>>>     	u8 set_config;
>>>>>> @@ -78,6 +81,9 @@ struct gpi_i2c_config {
>>>>>>     	u32 rx_len;
>>>>>>     	enum i2c_op op;
>>>>>>     	bool multi_msg;
>>>>>> +	bool shared_se;
>>>>>
>>>>> Looking at this why do you need this field? It can be internal to your
>>>>> i2c driver... Why not just set an enum for lock and use the values as
>>>>> lock/unlock/dont care and make the interface simpler. I see no reason to
>>>>> use three variables to communicate the info which can be handled in
>>>>> simpler way..?
>>>>>
>>>> Below was earlier reply to [PATCH V3, 2/4], please let me know if you have
>>>> any additional comment and need further clarifications.
>>>
>>> Looks like you misunderstood, the question is why do you need three
>>> variables to convey this info..? Use a single variable please
>> Yes, I think so. Please let me clarify.
>> First variable is a feature flag and it's required to be explicitly
>> mentioned by client (i2c/spi/etc) to GSI driver.
>>
>> Second and third, can be optimized to boolean so either first or last can be
>> passed.
>>
>> Please correct me or add simple change where you would like to make, i can
>> add that.
> 
> I though we could do with a single and derive
> 
Sure, so as mentioned in the other crypto BAM patch probably dmaengine.h 
can hold flag and that can add support for lock/unlock similar to that 
patch.
I just realized it from your shared patch. let me work internally with 
Md sadre and review. Thanks for the comment.
> Also, please see 20241212041639.4109039-3-quic_mdalam@quicinc.com, folks
> from same company should talk together on same solutions, please
> converge and come up with a single proposal which works for both drivers
> 
Sure