Re: [PATCH v2 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag

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

Thanks For your reviews,

On 9/10/2024 3:24 PM, Krzysztof Kozlowski wrote:
> On 10/09/2024 11:09, Mukesh Kumar Savaliya wrote:
>> Thanks Krzysztof.
>>
>> On 9/7/2024 2:34 PM, Krzysztof Kozlowski wrote:
>>> On 06/09/2024 21:14, Mukesh Kumar Savaliya wrote:
>>>> Adds qcom,shared-se flag usage. Use this when particular I2C serial
>>>> controller needs to be shared between two subsystems.
>>>
>>> <form letter>
>>> Please use scripts/get_maintainers.pl to get a list of necessary people
>>> and lists to CC (and consider --no-git-fallback argument). It might
>>> happen, that command when run on an older kernel, gives you outdated
>>> entries. Therefore please be sure you base your patches on recent Linux
>>> kernel.
>>>
>>> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
>>> people, so fix your workflow. Tools might also fail if you work on some
>>> ancient tree (don't, instead use mainline) or work on fork of kernel
>>> (don't, instead use mainline). Just use b4 and everything should be
>>> fine, although remember about `b4 prep --auto-to-cc` if you added new
>>> patches to the patchset.
>>> </form letter>
>>>
>>> You already got this comment, so how many times it has to be repeated?
>>> Your process is just wrong if you do not use the tools for this.
>>>
>> Sorry, I was already using scripts/get_maintainer.pl but i kept everyone
>> into To list (That's my mistake here). I shall keep maintainers in TO
>> list and rest in CC list.
> 
> No, To or Cc does not matter. Your list is just incomplete.
> 
Got it, sorry for the trouble. It seems i missed below 3 names adding 
into reviewers by copy paste mistake. I hope this makes it complete now 
and will add them in V3.

Bryan O'Donoghue" <bryan.odonoghue@linaro.org>
Krzysztof Kozlowski <krzk+dt@kernel.org>
Rob Herring <robh@kernel.org>
>>
>> Question: With <Form Letter> , are you asking to add letter in this
>> first patch ? I have cover letter, but it will get removed when patch
>> gets merged. Please help suggest and clarify.
> 
> No, it's just template. Form letter... I am just bored to repeat the
> same comment.
> 
Sorry for that. I hope i could catch now as per above missing list.
>>>
>>>>
>>>> SE = Serial Engine, meant for I2C controller here.
>>>> TRE = Transfer Ring Element, refers to Queued Descriptor.
>>>>
>>>> Example :
>>>> Two clients from different SS can share an I2C SE for same slave device
>>>
>>> What is SS?
>>>
>> SS = Subsystem (EE - Execution Environment, can be Apps
>> processor/TZ/Modem/ADSP etc). Let me add this too in next patch.
> 
> Yes, please explain in the binding itself.
> 
ok, Sure.
>>>> OR their owned slave devices.
>>>> Assume I2C Slave EEPROM device connected with I2C controller.
>>>> Each client from ADSP SS and APPS Linux SS can perform i2c transactions.
>>>> This gets serialized by lock TRE + DMA Transfers + Unlock TRE at HW level.
>>>>
>>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> index 9f66a3bb1f80..ae423127f736 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml
>>>> @@ -60,6 +60,10 @@ properties:
>>>>      power-domains:
>>>>        maxItems: 1
>>>>    
>>>> +  qcom,shared-se:
>>>> +    description: True if I2C needs to be shared between two or more subsystems.
>>>
>>> What is a subsystem? With commit msg I still do not understand this.
>> SS = Subsystem (EE - Execution Environment, can be Apps
>> processor/TZ/Modem/ADSP etc). Let me add EE too with full form.
>>> Maybe presence of hwlock defines it anyway, so this is redundant?
>> No, this flag is required. As hwlock comes into picture if this flag is
> 
> Flag is required? By what? Sorry, you push your downstream solution to us.
> 
Let me explain, Using this flag to take hwlock via TRE @ [PATCH v2 2/4]
We need this to lock SE protecting from other SS transfers until 
unlocked. Hence shared-se flag becomes a decision marker.
drivers/dma/qcom/gpi.c => gpi_create_i2c_tre()
+	/* create lock tre for first tranfser */
+	if (i2c->shared_se && i2c->first_msg) {

Question: what exactly you mean "Maybe presence of hwlock defines it 
anyway" ?
I am open to consider all upstream solutions, trying to understand your 
suggestions and comments.
>> defined. So flag is acting as a condition to take hwlock TRE
>> descriptor(transfer ring element). Hope i could answer your query.
> 
> Hm, not sure, maybe indeed hwlock would not be enough. However I think
> existing binding misses hwlock property.
> 
Let me clarify, you may help suggest further.
hwlock is a descriptor bit(TRE_I2C_LOCK).
"However I think  existing binding misses hwlock property"
Where shall i keep this hwlock property? what's the usage ?

> Best regards,
> Krzysztof
>