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

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

Hi Vinod, sorry for responding here much lately. I was completely away 
from this work for long, restarting now. Will close this actively 
following up here and I will upload V6 to share latest changes and cover 
letter details to help review ahead with context.

Thanks for your time and help review this ahead.

On 1/14/2025 2:48 PM, Mukesh Kumar Savaliya wrote:
> Hi Vinod,
> 
> On 12/26/2024 5:52 PM, Mukesh Kumar Savaliya wrote:
>>
>>
>> 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
>>>
> I have discussed with Md Sadre and tried to understand and utilize the 
> enum of lock and unlock in my changes. Below is the summary.
> 
> I can't use those lock and unlock enums here because it's required for 
> first and last message respectively. intermediate transfers will not use 
> anything. So we need to define one more enum like dma_ctrl_none.
> 
> if i create another internal parent structure having required 3 members, 
> then also it will need 3 child members. So i think current one looks 
> good to me.
> 
> Please help review and suggest if anything can be better here.
> 
I have added enum from gpi driver and set it from i2c driver. so GPI 
driver handles the action accordingly for lock/unlock. Let me know if 
this approach makes sense in the V6.

>> Sure
>>
>>
> 
>