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

[Mukesh Kumar Savaliya]

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@xxxxxxxxxx>
Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>
Rob Herring <robh@xxxxxxxxxx>
> > 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@xxxxxxxxxxx>
> > > > ---
> > > >    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