| Message ID | 20241113161413.3821858-2-quic_msavaliy@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable shared SE support over I2C | expand |
On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote: > Adds qcom,is-shared flag usage. Use this flag when I2C serial controller Doesn't match the property name. > needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. > > Two clients from different processors can share an I2C controller 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. > > Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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: What is 'se'? Is that defined somewhere? > + description: True if I2C controller is shared between two or more system processors. > + type: boolean > + > reg: > maxItems: 1 > > -- > 2.25.1 >
Thanks Rob for your review and comments ! On 11/15/2024 11:01 PM, Rob Herring wrote: > On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote: >> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller > > Doesn't match the property name. Sure, i need to change the name here as qcom,shared-se, will upload a new patch. > >> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >> >> Two clients from different processors can share an I2C controller 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. >> >> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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: > > What is 'se'? Is that defined somewhere? > SE is Serial Engine acting as I2C controller. Let me add second line for SE here also. It's mentioned in source code in Patch 3 where it's used. >>> True if serial engine is shared between multiprocessors OR Execution Environment. >> + description: True if I2C controller is shared between two or more system processors. >> + type: boolean >> + >> reg: >> maxItems: 1 >> >> -- >> 2.25.1 >>
On 17/11/2024 18:45, Mukesh Kumar Savaliya wrote: > Thanks Rob for your review and comments ! > > On 11/15/2024 11:01 PM, Rob Herring wrote: >> On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote: >>> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller >> >> Doesn't match the property name. > Sure, i need to change the name here as qcom,shared-se, will upload a > new patch. >> >>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >>> >>> Two clients from different processors can share an I2C controller 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. >>> >>> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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: >> >> What is 'se'? Is that defined somewhere? >> > SE is Serial Engine acting as I2C controller. Let me add second line for > SE here also. > > It's mentioned in source code in Patch 3 where it's used. > >>> True if serial engine is shared between multiprocessors OR > Execution Environment. You already got this comment: https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/#m79efdd1172631aca99a838b4bfe57943755701e3 ""se" is also not explained in the binding - please open it and look for such explanation." Further comments asked you to rephrase it. Did anything improve? No, nothing. You got comments, you ignore them and send the same. But most important: I keep repeating this over and over - NAK for some specific "shared-se" flag, different for each of your IP blocks. Come with something generic for entire qualcomm. There are few of such flags already and there are some patches adding it in different flavors. Get this consistent. NAK for this and v5 doing exactly theh same. Best regards, Krzysztof
Thanks Rob, Krzysztof ! On 11/25/2024 1:41 PM, Krzysztof Kozlowski wrote: > On 17/11/2024 18:45, Mukesh Kumar Savaliya wrote: >> Thanks Rob for your review and comments ! >> >> On 11/15/2024 11:01 PM, Rob Herring wrote: >>> On Wed, Nov 13, 2024 at 09:44:10PM +0530, Mukesh Kumar Savaliya wrote: >>>> Adds qcom,is-shared flag usage. Use this flag when I2C serial controller >>> >>> Doesn't match the property name. >> Sure, i need to change the name here as qcom,shared-se, will upload a >> new patch. >>> >>>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >>>> >>>> Two clients from different processors can share an I2C controller 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. >>>> >>>> Transfer gets serialized by Lock TRE + DMA xfer + 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..fe36938712f7 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: >>> >>> What is 'se'? Is that defined somewhere? >>> >> SE is Serial Engine acting as I2C controller. Let me add second line for >> SE here also. >> >> It's mentioned in source code in Patch 3 where it's used. >> >>> True if serial engine is shared between multiprocessors OR >> Execution Environment. > You already got this comment: > https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/#m79efdd1172631aca99a838b4bfe57943755701e3 > > ""se" is also not explained in the binding - please open it and look for > such explanation." > > Further comments asked you to rephrase it. Did anything improve? No, > nothing. > > You got comments, you ignore them and send the same. It's actually changed to is-shared flag and again renamed to shared-se based on the review comments. This went for correction for flag naming. Sorry for missing SE description into dt-bindings in the latest patch. I am adding it with more description. > > But most important: I keep repeating this over and over - NAK for some > specific "shared-se" flag, different for each of your IP blocks. Come > with something generic for entire qualcomm. There are few of such flags > already and there are some patches adding it in different flavors. > we do have SE (serial engine) which works for i2c, spi, uart, i3c. And SE is single HW entity as you are aware of. But I feel it makes sense to keep this flag name per SE and even for SPI OR I3C we should be using same flag name in DTSI. > Get this consistent. > > NAK for this and v5 doing exactly theh same. > Hope i meet expectations considering all your suggestions and past learning and not missing anything out of my mind. > Best regards, > Krzysztof
On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote: >> But most important: I keep repeating this over and over - NAK for some >> specific "shared-se" flag, different for each of your IP blocks. Come >> with something generic for entire qualcomm. There are few of such flags >> already and there are some patches adding it in different flavors. >> > we do have SE (serial engine) which works for i2c, spi, uart, i3c. And > SE is single HW entity as you are aware of. But I feel it makes sense to > keep this flag name per SE and even for SPI OR I3C we should be using > same flag name in DTSI. >> Get this consistent. >> >> NAK for this and v5 doing exactly theh same. >> > Hope i meet expectations considering all your suggestions and past > learning and not missing anything out of my mind. > Nothing from my comment above was resolved. I will NAK the next version as well for the same reasons. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml index 9f66a3bb1f80..fe36938712f7 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 controller is shared between two or more system processors. + type: boolean + reg: maxItems: 1
Adds qcom,is-shared flag usage. Use this flag when I2C serial controller needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. Two clients from different processors can share an I2C controller 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. Transfer gets serialized by Lock TRE + DMA xfer + 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(+)