| Message ID | 20241129144357.2008465-1-quic_msavaliy@quicinc.com |
|---|---|
| Headers | show |
| Series | Enable shared SE support over I2C | expand |
On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote: > Adds qcom,shared-se flag usage. Use this flag when I2C serial controller > needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. > > SE(Serial Engine HW controller acting as protocol master controller) is an > I2C controller. Basically a programmable SERDES(serializer/deserializer) > coupled with data DMA entity, capable in handling a bus protocol, and data > moves to/from system memory. > > 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> > --- > .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ > 1 file changed, 8 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..88682a333399 100644 > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > @@ -60,6 +60,14 @@ properties: > power-domains: > maxItems: 1 > > + qcom,shared-se: > + description: True if I2C controller is shared between two or more system processors. > + SE(Serial Engine HW controller working as protocol master controller) is an > + I2C controller. Basically, a programmable SERDES(serializer/deserializer) > + coupled with data DMA entity, capable in handling a bus protocol, and data > + moves to/from system memory. I replied why I NAK it. You did not really address my concerns, but replied with some generic statement. After that generic statement you gave me exactly 0 seconds to react and you sent v5. Really 0 seconds to respond to your comment, while you give yourself days to respond to my comments. This is not how it works. NAK Implement previous feedback. Don't send any new versions before you understand what you have to do and get some agreement with reviewers. Best regards, Krzysztof
On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote: > Adds qcom,shared-se flag usage. Use this flag when I2C serial controller > needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. > Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes your commit message should start with a description of your problem. "Add" isn't the right word to start a problem description with. > SE(Serial Engine HW controller acting as protocol master controller) is an > I2C controller. Basically a programmable SERDES(serializer/deserializer) "Basically"? > coupled with data DMA entity, capable in handling a bus protocol, and data > moves to/from system memory. > > 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. > The DeviceTree binding describes properties used to describe the hardware; your commit message describes what a SE is and that it can exist can exist in a configuration with multiple client etc etc. > Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level. > This isn't what this patch implements. It defines a property which when specified means to the OS that any DMA transfers should be performed using TRE lock/unlock operations. > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ > 1 file changed, 8 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..88682a333399 100644 > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > @@ -60,6 +60,14 @@ properties: > power-domains: > maxItems: 1 > > + qcom,shared-se: > + description: True if I2C controller is shared between two or more system processors. This attempts to describe the property. > + SE(Serial Engine HW controller working as protocol master controller) is an > + I2C controller. Basically, a programmable SERDES(serializer/deserializer) > + coupled with data DMA entity, capable in handling a bus protocol, and data > + moves to/from system memory. But this is basically just 4 lines of text expanding the acronym "se", but while it might give some insight into what this binding (the whole binding) is about, I'm afraid it doesn't add value to the understanding of the property... Regards, Bjorn > + type: boolean > + > reg: > maxItems: 1 > > -- > 2.25.1 >
Hi Krzysztof, On 11/29/2024 8:44 PM, Krzysztof Kozlowski wrote: > On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote: >> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller >> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >> >> SE(Serial Engine HW controller acting as protocol master controller) is an >> I2C controller. Basically a programmable SERDES(serializer/deserializer) >> coupled with data DMA entity, capable in handling a bus protocol, and data >> moves to/from system memory. >> >> 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> >> --- >> .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ >> 1 file changed, 8 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..88682a333399 100644 >> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> @@ -60,6 +60,14 @@ properties: >> power-domains: >> maxItems: 1 >> >> + qcom,shared-se: >> + description: True if I2C controller is shared between two or more system processors. >> + SE(Serial Engine HW controller working as protocol master controller) is an >> + I2C controller. Basically, a programmable SERDES(serializer/deserializer) >> + coupled with data DMA entity, capable in handling a bus protocol, and data >> + moves to/from system memory. > I replied why I NAK it. You did not really address my concerns, but > replied with some generic statement. After that generic statement you > gave me exactly 0 seconds to react and you sent v5. > Sorry for 0 seconds, i thought of addressing comment and uploading it new patch as i wanted to explain SE. whatever i have added for SE explanation is in qualcomm hardware programming guide document. > Really 0 seconds to respond to your comment, while you give yourself > days to respond to my comments. > > This is not how it works. > Sure, let me first conclude here what exactly should be done. > NAK > > Implement previous feedback. Don't send any new versions before you > understand what you have to do and get some agreement with reviewers. > Sure, this is definitely a good way. what did i do for previous comment ? I have opened SE and expanded, explained. which statement or explanation should i rephrase ? Is it description statement from this yaml file ? Could you please suggested better word instead of shared-se if this flag name is not suitable ? I could not get this ask - "There are few of such flags already and there are some patches adding it in different flavors." > Best regards, > Krzysztof
On 02/12/2024 05:00, Mukesh Kumar Savaliya wrote: > Hi Krzysztof, > > On 11/29/2024 8:44 PM, Krzysztof Kozlowski wrote: >> On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote: >>> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller >>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >>> >>> SE(Serial Engine HW controller acting as protocol master controller) is an >>> I2C controller. Basically a programmable SERDES(serializer/deserializer) >>> coupled with data DMA entity, capable in handling a bus protocol, and data >>> moves to/from system memory. >>> >>> 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> >>> --- >>> .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ >>> 1 file changed, 8 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..88682a333399 100644 >>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >>> @@ -60,6 +60,14 @@ properties: >>> power-domains: >>> maxItems: 1 >>> >>> + qcom,shared-se: >>> + description: True if I2C controller is shared between two or more system processors. >>> + SE(Serial Engine HW controller working as protocol master controller) is an >>> + I2C controller. Basically, a programmable SERDES(serializer/deserializer) >>> + coupled with data DMA entity, capable in handling a bus protocol, and data >>> + moves to/from system memory. >> I replied why I NAK it. You did not really address my concerns, but >> replied with some generic statement. After that generic statement you >> gave me exactly 0 seconds to react and you sent v5. >> > Sorry for 0 seconds, i thought of addressing comment and uploading it > new patch as i wanted to explain SE. whatever i have added for SE > explanation is in qualcomm hardware programming guide document. >> Really 0 seconds to respond to your comment, while you give yourself >> days to respond to my comments. >> >> This is not how it works. >> > Sure, let me first conclude here what exactly should be done. >> NAK >> >> Implement previous feedback. Don't send any new versions before you >> understand what you have to do and get some agreement with reviewers. >> > Sure, this is definitely a good way. what did i do for previous comment ? > I have opened SE and expanded, explained. > > which statement or explanation should i rephrase ? Is it description > statement from this yaml file ? Could you please suggested better word > instead of shared-se if this flag name is not suitable ? > > I could not get this ask - > "There are few of such flags already and there are some patches adding > it in different flavors." Come with one flag or enum, if needed, covering all your cases like this. Best regards, Krzysztof
Hi Bjorn, Thanks for the review ! On 11/30/2024 10:15 AM, Bjorn Andersson wrote: > On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote: >> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller >> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >> > > Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > your commit message should start with a description of your problem. > "Add" isn't the right word to start a problem description with. Problem statement i have explained in cover letter, let me add here too in V6. I thought same story gets repeated here. Will not start with Add, but problem statement and need of this flag. >> SE(Serial Engine HW controller acting as protocol master controller) is an >> I2C controller. Basically a programmable SERDES(serializer/deserializer) > > "Basically"? will remove this. > >> coupled with data DMA entity, capable in handling a bus protocol, and data >> moves to/from system memory. >> >> 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. >> > > The DeviceTree binding describes properties used to describe the > hardware; your commit message describes what a SE is and that it can > exist can exist in a configuration with multiple client etc etc. > I have explained the use of flag, and background surrounding to the feature. See the V4 and V5 and earlier, where i was required to explain and open up about "what is SE" ? Because of the SE word in flag name, i had to expand with explanation. >> Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level. >> > > This isn't what this patch implements. It defines a property which when > specified means to the OS that any DMA transfers should be performed > using TRE lock/unlock operations. I agree, it adds onto understanding about the flag feature. I can remove this statement in V6. Let me get complete agreement. > >> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> >> --- >> .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ >> 1 file changed, 8 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..88682a333399 100644 >> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> @@ -60,6 +60,14 @@ properties: >> power-domains: >> maxItems: 1 >> >> + qcom,shared-se: >> + description: True if I2C controller is shared between two or more system processors. > > This attempts to describe the property. I agree, thats why i limited but there was an ask to understand what is SE ? hence i added below. > >> + SE(Serial Engine HW controller working as protocol master controller) is an >> + I2C controller. Basically, a programmable SERDES(serializer/deserializer) >> + coupled with data DMA entity, capable in handling a bus protocol, and data >> + moves to/from system memory. > > But this is basically just 4 lines of text expanding the acronym "se", > but while it might give some insight into what this binding (the whole > binding) is about, I'm afraid it doesn't add value to the understanding > of the property... > ""se" is also not explained in the binding - please open it and look for such explanation." It was required to explain based on comment on V4, V5 hence i did. Please let me know if one line is enough to explain flag usage OR we need exact description from the hardware programming guide ? I will need to get agreement on this patch first and then upload V6. > Regards, > Bjorn > >> + type: boolean >> + >> reg: >> maxItems: 1 >> >> -- >> 2.25.1 >>
Thanks Krzysztof for giving clarity on ask and reviewing this change. On 12/2/2024 12:49 PM, Krzysztof Kozlowski wrote: > On 02/12/2024 05:00, Mukesh Kumar Savaliya wrote: >> Hi Krzysztof, >> >> On 11/29/2024 8:44 PM, Krzysztof Kozlowski wrote: >>> On 29/11/2024 15:43, Mukesh Kumar Savaliya wrote: >>>> Adds qcom,shared-se flag usage. Use this flag when I2C serial controller >>>> needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. >>>> >>>> SE(Serial Engine HW controller acting as protocol master controller) is an >>>> I2C controller. Basically a programmable SERDES(serializer/deserializer) >>>> coupled with data DMA entity, capable in handling a bus protocol, and data >>>> moves to/from system memory. >>>> >>>> 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> >>>> --- >>>> .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ >>>> 1 file changed, 8 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..88682a333399 100644 >>>> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >>>> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >>>> @@ -60,6 +60,14 @@ properties: >>>> power-domains: >>>> maxItems: 1 >>>> >>>> + qcom,shared-se: >>>> + description: True if I2C controller is shared between two or more system processors. >>>> + SE(Serial Engine HW controller working as protocol master controller) is an >>>> + I2C controller. Basically, a programmable SERDES(serializer/deserializer) >>>> + coupled with data DMA entity, capable in handling a bus protocol, and data >>>> + moves to/from system memory. >>> I replied why I NAK it. You did not really address my concerns, but >>> replied with some generic statement. After that generic statement you >>> gave me exactly 0 seconds to react and you sent v5. >>> >> Sorry for 0 seconds, i thought of addressing comment and uploading it >> new patch as i wanted to explain SE. whatever i have added for SE >> explanation is in qualcomm hardware programming guide document. >>> Really 0 seconds to respond to your comment, while you give yourself >>> days to respond to my comments. >>> >>> This is not how it works. >>> >> Sure, let me first conclude here what exactly should be done. >>> NAK >>> >>> Implement previous feedback. Don't send any new versions before you >>> understand what you have to do and get some agreement with reviewers. >>> >> Sure, this is definitely a good way. what did i do for previous comment ? >> I have opened SE and expanded, explained. >> >> which statement or explanation should i rephrase ? Is it description >> statement from this yaml file ? Could you please suggested better word >> instead of shared-se if this flag name is not suitable ? >> >> I could not get this ask - >> "There are few of such flags already and there are some patches adding >> it in different flavors." > > Come with one flag or enum, if needed, covering all your cases like this. > Let me explain, this feature is one of the additional software case adding on base protocol support. if we dont have more than one usecase or repurposing this feature, why do we need to add enums ? I see one flag gpi_mode but it's internal to driver not exposed to user or expose any usecase/feature. Below was our earlier context, just wanted to add for clarity. -- > 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. -- As we want to finalize agreement on this dt-bindings patch, will wait for agreement and confirmation before V6. > Best regards, > Krzysztof
On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote: >> >> Come with one flag or enum, if needed, covering all your cases like this. >> > Let me explain, this feature is one of the additional software case > adding on base protocol support. if we dont have more than one usecase > or repurposing this feature, why do we need to add enums ? I see one > flag gpi_mode but it's internal to driver not exposed to user or expose > any usecase/feature. > > Below was our earlier context, just wanted to add for clarity. > -- > > 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. You keep repeating the same. You won't receive any other answer. > > > 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. How flag per SE node is relevant? I did not ask to move the property. > > Please let me know your further suggestions. We do not talk about I2C or SPI here only. We talk about entire SoC. Since beginning. Find other patch proposals and align with rest of Qualcomm developers so that you come with only one definition for this feature/characteristic. Or do you want to say that I am free to NAK all further properties duplicating this one? Please confirm that you Qualcomm engineers understand the last statement and that every block will use se-shared, even if we speak about UFS for example. Best regards, Krzysztof
On 02/12/2024 12:04, Krzysztof Kozlowski wrote: > On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote: >>> >>> Come with one flag or enum, if needed, covering all your cases like this. >>> >> Let me explain, this feature is one of the additional software case >> adding on base protocol support. if we dont have more than one usecase >> or repurposing this feature, why do we need to add enums ? I see one >> flag gpi_mode but it's internal to driver not exposed to user or expose >> any usecase/feature. >> >> Below was our earlier context, just wanted to add for clarity. >> -- >> > 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. > > > You keep repeating the same. You won't receive any other answer. > >> > >> 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. > > > How flag per SE node is relevant? I did not ask to move the property. > >> >> Please let me know your further suggestions. > We do not talk about I2C or SPI here only. We talk about entire SoC. > Since beginning. Find other patch proposals and align with rest of > Qualcomm developers so that you come with only one definition for this > feature/characteristic. Or do you want to say that I am free to NAK all > further properties duplicating this one? > > Please confirm that you Qualcomm engineers understand the last statement > and that every block will use se-shared, even if we speak about UFS for > example. > I think I was pretty clear also 2 months ago what do I expect from this: https://lore.kernel.org/all/52f83419-cc5e-49f3-90a7-26a5b4ddd5a0@kernel.org/ I do not see this addressing qcom-wide way at all. Four new versions of patch and you still did not address first fedback you got. Best regards, Krzysztof
On 12/2/2024 4:34 PM, Krzysztof Kozlowski wrote: > On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote: >>> >>> Come with one flag or enum, if needed, covering all your cases like this. >>> >> Let me explain, this feature is one of the additional software case >> adding on base protocol support. if we dont have more than one usecase >> or repurposing this feature, why do we need to add enums ? I see one >> flag gpi_mode but it's internal to driver not exposed to user or expose >> any usecase/feature. >> >> Below was our earlier context, just wanted to add for clarity. >> -- >> > 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. > > > You keep repeating the same. You won't receive any other answer. > So far i was in context to SEs. I am not sure in qualcomm SOC all cores supporting this feature and if it at all it supports, it may have it's own mechanism then what is followed in SE IP. I was probably thinking on my owned IP core hence i was revolving around. Hope this dt-binding i can conclude somewhere by seeking answer from other IP core owners within qualcomm. >> > >> 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. > > > How flag per SE node is relevant? I did not ask to move the property. > >> >> Please let me know your further suggestions. > We do not talk about I2C or SPI here only. We talk about entire SoC. > Since beginning. Find other patch proposals and align with rest of > Qualcomm developers so that you come with only one definition for this > feature/characteristic. Or do you want to say that I am free to NAK all > further properties duplicating this one? > > Please confirm that you Qualcomm engineers understand the last statement > and that every block will use se-shared, even if we speak about UFS for > example. This UFS word atleast makes me understand and gave me clarity that i need to talk to different IP owners within qualcomm and get an agreement for my i2c feature. I am not sure if there exist an usecase the way we are sharing for i2c. Also i don't know how we can make similar description if different cores and functionality are different. If you have heard from any other IP core, please keep some usecases/IP names. Since This demands internal discussion, so give me time to conclude how the IPs are shared and is it the similar to what i have developed here for I2C. (sorry that so far i was in context to my SE protocols/ IPs only). > > Best regards, > Krzysztof
On 2.12.2024 1:55 PM, Mukesh Kumar Savaliya wrote: > > > On 12/2/2024 4:34 PM, Krzysztof Kozlowski wrote: >> On 02/12/2024 11:38, Mukesh Kumar Savaliya wrote: >>>> >>>> Come with one flag or enum, if needed, covering all your cases like this. >>>> >>> Let me explain, this feature is one of the additional software case >>> adding on base protocol support. if we dont have more than one usecase >>> or repurposing this feature, why do we need to add enums ? I see one >>> flag gpi_mode but it's internal to driver not exposed to user or expose >>> any usecase/feature. >>> >>> Below was our earlier context, just wanted to add for clarity. >>> -- >>> > 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. >> >> >> You keep repeating the same. You won't receive any other answer. >> > So far i was in context to SEs. I am not sure in qualcomm SOC all cores supporting this feature and if it at all it supports, it may have it's own mechanism then what is followed in SE IP. I was probably thinking on my owned IP core hence i was revolving around. > > Hope this dt-binding i can conclude somewhere by seeking answer from other IP core owners within qualcomm. >>> > >>> 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. >> >> >> How flag per SE node is relevant? I did not ask to move the property. >> >>> >>> Please let me know your further suggestions. >> We do not talk about I2C or SPI here only. We talk about entire SoC. >> Since beginning. Find other patch proposals and align with rest of >> Qualcomm developers so that you come with only one definition for this >> feature/characteristic. Or do you want to say that I am free to NAK all >> further properties duplicating this one? I'm not sure a single property name+description can fit all possible cases here. The hardware being "shared" can mean a number of different things, with some blocks having hardware provisions for that, while others may have totally none and rely on external mechanisms (e.g. a shared memory buffer) to indicate whether an external entity manages power to them. Even here, I'm not particularly sure whether dt is the right place. Maybe we could think about checking for clock_is_enabled()? That's just an idea, as it may fall apart if CCF gets a .sync_state impl. Konrad
On Mon, Dec 02, 2024 at 04:08:32PM +0530, Mukesh Kumar Savaliya wrote: > Hi Bjorn, Thanks for the review ! > > On 11/30/2024 10:15 AM, Bjorn Andersson wrote: > > On Fri, Nov 29, 2024 at 08:13:54PM +0530, Mukesh Kumar Savaliya wrote: > > > Adds qcom,shared-se flag usage. Use this flag when I2C serial controller > > > needs to be shared in multiprocessor system(APPS,Modem,ADSP) environment. > > > > > > > Per https://docs.kernel.org/process/submitting-patches.html#describe-your-changes > > your commit message should start with a description of your problem. > > "Add" isn't the right word to start a problem description with. > Problem statement i have explained in cover letter, let me add here too in > V6. I thought same story gets repeated here. Will not start with Add, but > problem statement and need of this flag. The cover-letter is generally not included in the git history, so that explanation would be lost on future readers. > > > SE(Serial Engine HW controller acting as protocol master controller) is an > > > I2C controller. Basically a programmable SERDES(serializer/deserializer) > > > > "Basically"? > will remove this. > > > > > coupled with data DMA entity, capable in handling a bus protocol, and data > > > moves to/from system memory. > > > > > > 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. > > > > > > > The DeviceTree binding describes properties used to describe the > > hardware; your commit message describes what a SE is and that it can > > exist can exist in a configuration with multiple client etc etc. > > > I have explained the use of flag, and background surrounding to the feature. > See the V4 and V5 and earlier, where i was required to explain and open up > about "what is SE" ? > Because of the SE word in flag name, i had to expand with explanation. > > > Transfer gets serialized by Lock TRE + DMA xfer + Unlock TRE at HW level. > > > > > > > This isn't what this patch implements. It defines a property which when > > specified means to the OS that any DMA transfers should be performed > > using TRE lock/unlock operations. > I agree, it adds onto understanding about the flag feature. I can remove > this statement in V6. Let me get complete agreement. I think the understanding is necessary, but that the wording should be different. Imaging you're implementing MukeshOS and reading this binding document to understand what you're expected to do in your I2C driver when you see this property. Similarly, the binding document should be sufficiently clear such that a newly hired colleague of ours would understand if they should put this property or not in the dts file they are writing. > > > > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > > > --- > > > .../devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++++++ > > > 1 file changed, 8 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..88682a333399 100644 > > > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > > > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > > > @@ -60,6 +60,14 @@ properties: > > > power-domains: > > > maxItems: 1 > > > + qcom,shared-se: > > > + description: True if I2C controller is shared between two or more system processors. > > > > This attempts to describe the property. > I agree, thats why i limited but there was an ask to understand what is SE ? > hence i added below. > > > > > + SE(Serial Engine HW controller working as protocol master controller) is an > > > + I2C controller. Basically, a programmable SERDES(serializer/deserializer) > > > + coupled with data DMA entity, capable in handling a bus protocol, and data > > > + moves to/from system memory. > > > > But this is basically just 4 lines of text expanding the acronym "se", > > but while it might give some insight into what this binding (the whole > > binding) is about, I'm afraid it doesn't add value to the understanding > > of the property... > > > ""se" is also not explained in the binding - please open it and look for > such explanation." > > It was required to explain based on comment on V4, V5 hence i did. Please > let me know if one line is enough to explain flag usage OR we need exact > description from the hardware programming guide ? > What I'm saying is that this binding is for the serial engine, if you need to describe what SE or a serial engine is you should do that in the top-level description, not within one of the properties (or in a possible future repeat that explanation in multiple different properties). > I will need to get agreement on this patch first and then upload V6. > Sounds good. Regards, Bjorn > > Regards, > > Bjorn > > > > > + type: boolean > > > + > > > reg: > > > maxItems: 1 > > > -- > > > 2.25.1 > > >
QUP based I2C GENI controller driver doesn't support controller sharing between two processors (E.g. APPS and DSP) running same or different OS. For example, two I2C clients (one from ADSP and another from APPS) want to communicate with EEPROM connected over I2C without any bus level disturbance during transfer. This Series adds support to share QUP (Qualcomm Unified peripheral) based I2C SE (Serial Engine) controller between two or more processors (Apps/ADSP etc). Each system processor is having its own dedicated GPII(General Purpose Interface Instance) acting as pipe between SE and GSI(Generic SW- Interface) DMA HW engine. Hence the sharing of I2C is possible only in GPI mode, not with FIFO/CPU DMA mode. Each Processor subsystem must acquire Lock over the controller so that it gets uninterrupted control till it unlocks the SE. Generally, GPIOs are being unconfigured during It also makes sure the commonly shared TLMM GPIOs are not touched which can impact other subsystem or cause any interruption. suspend time. Transfer from each processor gets serialized by lock TRE + Transfers + Unlock TRE at HW level. GSI DMA engine is capable to perform requested transfer operations from any of the I2C client in a seamless way and its transparent to the subsystems. Make sure to enable “qcom,is-shared” flag only while enabling this feature. I2C client should add in its respective parent node. Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> ---- Link to V4 : https://lore.kernel.org/lkml/20241113161413.3821858-1-quic_msavaliy@quicinc.com/ Changes in V5: - Corrected name as qcom,shared-se instead of qcom,is-shared. - Added description for the SE acronyms into yaml file and commit log. - Renamed TRE_I2C_UNLOCK to TRE_UNLOCK being generic. - Log an error and return if non GPI mode goes into shared usecase. --- Link to V3: https://lore.kernel.org/lkml/20240927063108.2773304-4-quic_msavaliy@quicinc.com/T/ Changes in V4: - Fixed Typo to dt-bindings in subject line of PATCH 1. - Replaced SS (subsystem) as multiprocessor as per Bryan's suggestions. - Replied to Krzysztof's comments and replaced SS with Multiprocessor system. - Removed Abbreviations and also bullet point list from PATCH 1. - Changed feature flag name from qcom,shared-se to qcom,is-shared. - Removed bullet points from example of usecase and explained in paragraph. - Changed title suffix to dmaengine from dma for Patch 2. - Rename TRE_I2C_LOCK to TRE_LOCK in PATCH 2. - Enhanced comments about not modifying the pin states on shared SE for PATCH 3. - Enhanced shared_geni_se struct member explanation as per Bjorn's comment in PATCH 3. - Moved GPIO unconfiguration description from patch 4 to patch 3 as pointed by Bjorn. - Removed debug log which was unrelated to this feature change. - Added usecase exmaple of shared SE in commit log. --- Link to V2: https://lore.kernel.org/lkml/a88a16ff-3537-4396-b2ea-4ba02b4850e9@quicinc.com/T/ Changes in V3: - Added missing maintainers which i forgot to add. - Add cover letter with description of SS and EE for dt-bindings patch. - Added acronyms expansion to commit log. - [PATCH v2 3/4] : Removed exported symbol geni_se_clks_off(). Instead added changes to bypass pinctrl sleep configuration from geni_se_resources_off() function. - Changed title name of [PATCH v2 3/4] to reflect the suggested changes. - [PATCH v2 4/4] kept geni_i2c_runtime_suspend() as is and removed explicit call to geni_se_clks_off(). - Removed is_shared variable from i2c driver and instead used common shared_geni_se variable from qcom-geni-se.h so that other protocols can also extend for similar feature. - I2C driver log changed from dev_err() to dev_dbg() for timeout. - set gpi_mode = true if shared_geni_se is set for this usecase. Enhanced comments around code and commit log. --- Link to V1: https://lore.kernel.org/lkml/cb7613d0-586e-4089-a1b6-2405f4dc4883@quicinc.com/T/ Changes in V2: - Enhanced commit log grammatically for PATCH v1 3/4 as suggested by Bryan. - Updated Cover letter along with acronyms expansion. - Added maintainers list from other subsystems for review, which was missing. Thanks to Krzysztof for pointing out. - Added cover letter with an example of Serial Engine sharing. - Addressed review comments for all the patches. --- Mukesh Kumar Savaliya (4): dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag dmaengine: gpi: Add Lock and Unlock TRE support to access I2C exclusively soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems .../bindings/i2c/qcom,i2c-geni-qcom.yaml | 8 ++++ drivers/dma/qcom/gpi.c | 37 ++++++++++++++++++- drivers/i2c/busses/i2c-qcom-geni.c | 22 +++++++++-- drivers/soc/qcom/qcom-geni-se.c | 13 +++++-- include/linux/dma/qcom-gpi-dma.h | 6 +++ include/linux/soc/qcom/geni-se.h | 3 ++ 6 files changed, 81 insertions(+), 8 deletions(-)