| Message ID | 20240927063108.2773304-2-quic_msavaliy@quicinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable shared SE support over I2C | expand |
On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote: > Adds qcom,shared-se flag usage. Use this when particular I2C serial > controller needs to be shared between two subsystems. > > SE = Serial Engine, meant for I2C controller here. > TRE = Transfer Ring Element, refers to Queued Descriptor. > SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). > > Example : > Two clients from different SS can share an I2C SE for same slave device > 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..3b9b20a0edff 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(SS). The "SS" and subsystem should be explained in the binding. Please do not use some qcom-specific abbreviations here, but explain exactly, e.g. processors like application processor and DSP. "se" is also not explained in the binding - please open it and look for such explanation. This all should be rephrased to make it clear... We talked about this and I do not see much of improvements except commit msg, so we are making circles. I don't know, get someone internally to help you in upstreaming this. Is sharing of IP blocks going to be also for other devices? If yes, then this should be one property for all Qualcomm devices. If not, then be sure that this is the case because I will bring it up if you come with one more solution for something else. Best regards, Krzysztof
On 27/09/2024 08:31, Mukesh Kumar Savaliya wrote: > Adds qcom,shared-se flag usage. Use this when particular I2C serial > controller needs to be shared between two subsystems. > Also, fix the typo in subject prefix. It is dt-bindings. Best regards, Krzysztof
On 27.09.2024 11:24 AM, Krzysztof Kozlowski wrote: > On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote: >> Adds qcom,shared-se flag usage. Use this when particular I2C serial >> controller needs to be shared between two subsystems. >> >> SE = Serial Engine, meant for I2C controller here. >> TRE = Transfer Ring Element, refers to Queued Descriptor. >> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). >> >> Example : >> Two clients from different SS can share an I2C SE for same slave device >> 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..3b9b20a0edff 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(SS). > > The "SS" and subsystem should be explained in the binding. Please do not > use some qcom-specific abbreviations here, but explain exactly, e.g. > processors like application processor and DSP. > > "se" is also not explained in the binding - please open it and look for > such explanation. > > This all should be rephrased to make it clear... We talked about this > and I do not see much of improvements except commit msg, so we are > making circles. I don't know, get someone internally to help you in > upstreaming this. > > Is sharing of IP blocks going to be also for other devices? If yes, then > this should be one property for all Qualcomm devices. If not, then be > sure that this is the case because I will bring it up if you come with > one more solution for something else. As far as I understand, everything that's not protocol-specific (in this case it would be I2C tunables etc.) is common across all protocols supported by the serial engine. Konrad
On 27/09/2024 13:20, Konrad Dybcio wrote: > On 27.09.2024 11:24 AM, Krzysztof Kozlowski wrote: >> On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote: >>> Adds qcom,shared-se flag usage. Use this when particular I2C serial >>> controller needs to be shared between two subsystems. >>> >>> SE = Serial Engine, meant for I2C controller here. >>> TRE = Transfer Ring Element, refers to Queued Descriptor. >>> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). >>> >>> Example : >>> Two clients from different SS can share an I2C SE for same slave device >>> 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..3b9b20a0edff 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(SS). >> >> The "SS" and subsystem should be explained in the binding. Please do not >> use some qcom-specific abbreviations here, but explain exactly, e.g. >> processors like application processor and DSP. >> >> "se" is also not explained in the binding - please open it and look for >> such explanation. >> >> This all should be rephrased to make it clear... We talked about this >> and I do not see much of improvements except commit msg, so we are >> making circles. I don't know, get someone internally to help you in >> upstreaming this. >> >> Is sharing of IP blocks going to be also for other devices? If yes, then >> this should be one property for all Qualcomm devices. If not, then be >> sure that this is the case because I will bring it up if you come with >> one more solution for something else. > > As far as I understand, everything that's not protocol-specific (in > this case it would be I2C tunables etc.) is common across all > protocols supported by the serial engine. Yeah, but I also think about other things like clock controllers, TLMMs, MMUs and so on. Each of them will get a new "qcom,shared-xxx" property? I expect Mukesh to solve it in qcom-wide way, not only his one problem. Best regards, Krzysztof
On Fri, Sep 27, 2024 at 12:01:05PM GMT, Mukesh Kumar Savaliya wrote: > Adds qcom,shared-se flag usage. Use this when particular I2C serial > controller needs to be shared between two subsystems. > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > SE = Serial Engine, meant for I2C controller here. > TRE = Transfer Ring Element, refers to Queued Descriptor. > SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). Expressing yourself in terms of abbreviations just makes the text harder to read. The dictionary is nice, but I don't see that it adds value to introduce these abbreviations with the reader. > > Example : > Two clients from different SS can share an I2C SE for same slave device > 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. > Don't describe your problem using a bullet-point list. You should be able to express it in a flowing English sentence/paragraph. > 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..3b9b20a0edff 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(SS). I see no value in establishing the "SS" abbreviation here, what would be useful is to write this sentence such that it establishes the "SE" abbreviation (to avoid having to expand the property). On the other hand, it's a boolean property in a serial-engine node, so I don't know if it's worth repeating "se" here. "qcom,is-shared" sounds like a better boolean in a se-node. Regards, Bjorn > + type: boolean > + > reg: > maxItems: 1 > > -- > 2.25.1 >
Thanks Krzysztof! On 9/27/2024 3:31 PM, Krzysztof Kozlowski wrote: > On 27/09/2024 08:31, Mukesh Kumar Savaliya wrote: >> Adds qcom,shared-se flag usage. Use this when particular I2C serial >> controller needs to be shared between two subsystems. >> > > Also, fix the typo in subject prefix. It is dt-bindings. Sure, Done. > > Best regards, > Krzysztof >
On 9/27/2024 2:54 PM, Krzysztof Kozlowski wrote: > On Fri, Sep 27, 2024 at 12:01:05PM +0530, Mukesh Kumar Savaliya wrote: >> Adds qcom,shared-se flag usage. Use this when particular I2C serial >> controller needs to be shared between two subsystems. >> >> SE = Serial Engine, meant for I2C controller here. >> TRE = Transfer Ring Element, refers to Queued Descriptor. >> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). >> >> Example : >> Two clients from different SS can share an I2C SE for same slave device >> 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..3b9b20a0edff 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(SS). > > The "SS" and subsystem should be explained in the binding. Please do not > use some qcom-specific abbreviations here, but explain exactly, e.g. > processors like application processor and DSP. > > "se" is also not explained in the binding - please open it and look for > such explanation. Sure, i thought cover letter explanation is good enough. I will add it per patch as cover letter will not be visible and go away after merge. > > This all should be rephrased to make it clear... We talked about this > and I do not see much of improvements except commit msg, so we are > making circles. I don't know, get someone internally to help you in > upstreaming this. Let me retry to make it better. Will make SS (subsystem) to system processor (can be APPS or DSP OR any other). > > Is sharing of IP blocks going to be also for other devices? If yes, then > this should be one property for all Qualcomm devices. If not, then be > sure that this is the case because I will bring it up if you come with > one more solution for something else. > IP blocks like SE can be shared. Here we are talking about I2C sharing. In future it can be SPI sharing. But design wise it fits better to add flag per SE node. Same we shall be adding for SPI too in future. Please let me know your further suggestions. > Best regards, > Krzysztof >
Thanks Bjorn ! On 9/30/2024 8:50 AM, Bjorn Andersson wrote: > On Fri, Sep 27, 2024 at 12:01:05PM GMT, Mukesh Kumar Savaliya wrote: >> Adds qcom,shared-se flag usage. Use this when particular I2C serial >> controller needs to be shared between two subsystems. >> > > https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > >> SE = Serial Engine, meant for I2C controller here. >> TRE = Transfer Ring Element, refers to Queued Descriptor. >> SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). > > Expressing yourself in terms of abbreviations just makes the text harder > to read. The dictionary is nice, but I don't see that it adds value to > introduce these abbreviations with the reader. Sure, thought it will ease the explanations of abbreviations. I learnt that not to use it and directly use complete name in opensource way. > >> >> Example : >> Two clients from different SS can share an I2C SE for same slave device >> 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. >> > > Don't describe your problem using a bullet-point list. You should be > able to express it in a flowing English sentence/paragraph. Sure Bjorn. > >> 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..3b9b20a0edff 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(SS). > > I see no value in establishing the "SS" abbreviation here, what would be > useful is to write this sentence such that it establishes the "SE" > abbreviation (to avoid having to expand the property). > > On the other hand, it's a boolean property in a serial-engine node, so > I don't know if it's worth repeating "se" here. "qcom,is-shared" sounds > like a better boolean in a se-node. > Sure, Done. I thought shared-se will make it understand that SE is shared. I will modify to qcom,is-shared as recommended. Also i am removing SS and make it multiprocessor as asked by Bryan. > Regards, > Bjorn > >> + type: boolean >> + >> reg: >> maxItems: 1 >> >> -- >> 2.25.1 >>
diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml index 9f66a3bb1f80..3b9b20a0edff 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(SS). + type: boolean + reg: maxItems: 1
Adds qcom,shared-se flag usage. Use this when particular I2C serial controller needs to be shared between two subsystems. SE = Serial Engine, meant for I2C controller here. TRE = Transfer Ring Element, refers to Queued Descriptor. SS = Subsystems (APPS processor, Modem, TZ, ADSP etc). Example : Two clients from different SS can share an I2C SE for same slave device 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(+)