| Message ID | 20241129144357.2008465-2-quic_msavaliy@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Enable shared SE support over I2C | expand |
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 > > >
On Tue, Dec 10, 2024 at 01:38:28PM +0100, Konrad Dybcio wrote: > > > On 12/10/24 13:05, Krzysztof Kozlowski wrote: > > On 10/12/2024 12:53, Krzysztof Kozlowski wrote: > > > > > > 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 > > > > > > > > > > Existing property does not explain anything more, either. To recap - > > > > > this block is SE and property is named "se-shared", so basically it is > > > > > equal to just "shared". "shared" is indeed quite vague, so I was > > > > > expecting some wider work here. > > > > > > > > > > > > > > > > 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. > > > > > > > > > > We have properties for that too. Qualcomm SoCs need once per year for > > > > > such shared properties. BAM has two or three. IPA has two. There are > > > > > probably even more blocks which I don't remember now. > > > > > > > > So, the problem is "driver must not toggle GPIO states", because > > > > "the bus controller must not be muxed away from the endpoint". > > > > You can come up with a number of similar problems by swapping out > > > > the quoted text. > > > > > > > > We can either describe what the driver must do (A), or what the > > > > reason for it is (B). > > > > > > > > > > > > If we go with A, we could have a property like: > > > > > > > > &i2c1 { > > > > externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)> > > > > }; > > > > > > > > which would be a generic list of things that the OS would have to > > > > tiptoe around, fitting Linux's framework split quite well > > > > > > > > > > > > > > > > or if we go with B, we could add a property like: > > > > > > > > &i2c1 { > > > > qcom,shared-controller; > > > > }; > > > > > > > > which would hide the implementation details into the driver > > > > > > > > I could see both approaches having their place, but in this specific > > > > instance I think A would be more fitting, as the problem is quite > > > > simple. > > > > > > > > > The second is fine with me, maybe missing information about "whom" do > > > you share it with. Or maybe we get to the point that all this is > > > specific to SoC, thus implied by compatible and we do not need > > > downstream approach (another discussion in USB pushed by Qcom: I want > > > one compatible and 1000 properties). > > > > > > I really wished Qualcomm start reworking their bindings before they are > > > being sent upstream to match standard DT guidelines, not downstream > > > approach. Somehow these hundreds reviews we give could result in new > > > patches doing things better, not just repeating the same issues. > > > > This is BTW v5, with all the same concerns from v1 and still no answers > > in commit msg about these concerns. Nothing explained in commit msg > > which hardware needs it or why the same SoC have it once shared, once > > not (exclusive). Basically there is nothing here corresponding to any > > real product, so since five versions all this for me is just copy-paste > > from downstream approach. > > So since this is a software contract and not a hardware > feature, this is not bound to any specific SoC or "firmware", > but rather to what runs on other cores (e.g. DSPs, MCUs spread > across the SoC or in a different software world, like TZ). > I don't think this is a reasonable distinction, the DeviceTree must describe the interfaces/environment that the OS is to operate in. Claiming that certain properties of that world directly or indirectly comes from (static) "software choices" would make the whole concept of DeviceTree useless. The fact that a serial engine is shared, or not, is a static property of the firmware for a given board, no different from "i2c1 being accessible by this OS or not" or the fact that i2c1 is actually implement I2C and not SPI (i.e. should this node be enabled in the DeviceTree passed to the OS or not). That said, the commit message still doesn't clearly describe the system design or when this property should be set or not, which is what Krzysztof has been asking for multiple times. Let's circle back and help Mukesh rewrite the commit message such that it clearly documents the problem being solved. > Specifying the specific intended use would be helpful though, > indeed. > > Let's see if we can somehow make this saner. > > > Mukesh, do we have any spare registers that we could use to > indicate that a given SE is shared? Preferably within the > SE's register space itself. The bootloader or another entity > (DSP or what have you) would then set that bit before Linux > runs and we could skip the bindings story altogether. > > It would need to be reserved on all SoCs though (future and > past), to make sure the contract is always held up, but I > think finding a persistent bit that has never been used > shouldn't be impossible. > Let's not invent a custom one-off "hardware description" passing interface. Regards, Bjorn > Konrad
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. + type: boolean + reg: maxItems: 1
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(+)