| Message ID | 20240903220240.2594102-17-quic_nkela@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2,01/21] dt-bindings: arm: qcom: add the SoC ID for SA8255P | expand |
On Tue, Sep 03, 2024 at 03:02:35PM -0700, Nikunj Kela wrote: > Add compatible representing spi support on SA8255p. > > Clocks and interconnects are being configured in firmware VM > on SA8255p platform, therefore making them optional. > Please use standard email subjects, so with the PATCH keyword in the title. helps here to create proper versioned patches. Another useful tool is b4. Skipping the PATCH keyword makes filtering of emails more difficult thus making the review process less convenient. > CC: Praveen Talari <quic_ptalari@quicinc.com> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > --- > .../bindings/spi/qcom,spi-geni-qcom.yaml | 60 +++++++++++++++++-- > 1 file changed, 56 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml > index 2e20ca313ec1..75b52c0a7440 100644 > --- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml > +++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml > @@ -25,10 +25,45 @@ description: > > allOf: > - $ref: /schemas/spi/spi-controller.yaml# > + - if: > + properties: > + compatible: > + contains: > + const: qcom,sa8255p-geni-spi Not much improved. All my previous (v1) and other patch (i2c) comments apply. > + then: > + required: > + - power-domains > + - power-domain-names > + > + properties: > + power-domains: > + minItems: 2 > + > + else: > + required: > + - clocks > + - clock-names > + > + properties: > + power-domains: > + maxItems: 1 > + > + interconnects: > + minItems: 2 > + maxItems: 3 > + > + interconnect-names: > + minItems: 2 > + items: > + - const: qup-core > + - const: qup-config > + - const: qup-memory > > properties: > compatible: > - const: qcom,geni-spi > + enum: > + - qcom,geni-spi > + - qcom,sa8255p-geni-spi You have entire commit msg to explain why this device's programming model is not compatible with existing generic compatible which must cover all variants (because it is crazy generic). Best regards, Krzysztof
On 04/09/2024 00:02, Nikunj Kela wrote: > Add compatible representing spi support on SA8255p. > > Clocks and interconnects are being configured in firmware VM > on SA8255p platform, therefore making them optional. > > CC: Praveen Talari <quic_ptalari@quicinc.com> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> Also this is incomplete - adding compatible without driver change is not expected. It cannot even work. Best regards, Krzysztof
On 9/3/2024 11:34 PM, Krzysztof Kozlowski wrote: > On Tue, Sep 03, 2024 at 03:02:35PM -0700, Nikunj Kela wrote: >> Add compatible representing spi support on SA8255p. >> >> Clocks and interconnects are being configured in firmware VM >> on SA8255p platform, therefore making them optional. >> > Please use standard email subjects, so with the PATCH keyword in the > title. helps here to create proper versioned patches. Where did I miss PATCH keyword in the subject here? It says "[PATCH v2 16/21] dt-bindings: spi: document support for SA8255p" > Another useful tool is b4. Skipping the PATCH keyword makes filtering of > emails more difficult thus making the review process less convenient. > > >> CC: Praveen Talari <quic_ptalari@quicinc.com> >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> >> --- >> .../bindings/spi/qcom,spi-geni-qcom.yaml | 60 +++++++++++++++++-- >> 1 file changed, 56 insertions(+), 4 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml >> index 2e20ca313ec1..75b52c0a7440 100644 >> --- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml >> +++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml >> @@ -25,10 +25,45 @@ description: >> >> allOf: >> - $ref: /schemas/spi/spi-controller.yaml# >> + - if: >> + properties: >> + compatible: >> + contains: >> + const: qcom,sa8255p-geni-spi > Not much improved. All my previous (v1) and other patch (i2c) comments > apply. >> + then: >> + required: >> + - power-domains >> + - power-domain-names >> + >> + properties: >> + power-domains: >> + minItems: 2 >> + >> + else: >> + required: >> + - clocks >> + - clock-names >> + >> + properties: >> + power-domains: >> + maxItems: 1 >> + >> + interconnects: >> + minItems: 2 >> + maxItems: 3 >> + >> + interconnect-names: >> + minItems: 2 >> + items: >> + - const: qup-core >> + - const: qup-config >> + - const: qup-memory >> >> properties: >> compatible: >> - const: qcom,geni-spi >> + enum: >> + - qcom,geni-spi >> + - qcom,sa8255p-geni-spi > You have entire commit msg to explain why this device's programming > model is not compatible with existing generic compatible which must > cover all variants (because it is crazy generic). > > Best regards, > Krzysztof I will put more details in the description of the patch, though, I had put the description in the cover letter for this entire series. >
On 9/4/2024 12:48 AM, Krzysztof Kozlowski wrote: > On 04/09/2024 00:02, Nikunj Kela wrote: >> Add compatible representing spi support on SA8255p. >> >> Clocks and interconnects are being configured in firmware VM >> on SA8255p platform, therefore making them optional. >> >> CC: Praveen Talari <quic_ptalari@quicinc.com> >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > Also this is incomplete - adding compatible without driver change is not > expected. It cannot even work. > > Best regards, > Krzysztof Link for CLO branch is provided in I2C patch series. The driver changes will soon follow.
On 04/09/2024 14:48, Nikunj Kela wrote: > > On 9/3/2024 11:34 PM, Krzysztof Kozlowski wrote: >> On Tue, Sep 03, 2024 at 03:02:35PM -0700, Nikunj Kela wrote: >>> Add compatible representing spi support on SA8255p. >>> >>> Clocks and interconnects are being configured in firmware VM >>> on SA8255p platform, therefore making them optional. >>> >> Please use standard email subjects, so with the PATCH keyword in the >> title. helps here to create proper versioned patches. > Where did I miss PATCH keyword in the subject here? It says "[PATCH v2 > 16/21] dt-bindings: spi: document support for SA8255p" Oh, wrong template. It was about spi prefix, should be this one: Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. For bindings, the preferred subjects are explained here: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters >> Best regards, Krzysztof
On 9/4/2024 6:21 AM, Krzysztof Kozlowski wrote: > On 04/09/2024 14:48, Nikunj Kela wrote: >> On 9/3/2024 11:34 PM, Krzysztof Kozlowski wrote: >>> On Tue, Sep 03, 2024 at 03:02:35PM -0700, Nikunj Kela wrote: >>>> Add compatible representing spi support on SA8255p. >>>> >>>> Clocks and interconnects are being configured in firmware VM >>>> on SA8255p platform, therefore making them optional. >>>> >>> Please use standard email subjects, so with the PATCH keyword in the >>> title. helps here to create proper versioned patches. >> Where did I miss PATCH keyword in the subject here? It says "[PATCH v2 >> 16/21] dt-bindings: spi: document support for SA8255p" > Oh, wrong template. It was about spi prefix, should be this one: Sorry, didn't realize SPI uses different subject format than other subsystems. Will fix in v3. Thanks > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. For bindings, the preferred subjects are > explained here: > https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters > > Best regards, > Krzysztof >
> Sorry, didn't realize SPI uses different subject format than other > subsystems. Will fix in v3. Thanks Each subsystem is free to use its own form. e.g for netdev you will want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: This is another reason why you should be splitting these patches per subsystem, and submitting both the DT bindings and the code changes as a two patch patchset. You can then learn how each subsystem names its patches. Please pick one victim subsystem and work on the patches for just that subsystem. Once you have them correct, you can use everything you learned to fixup all your other patches, one by one. Andrew
On 9/4/2024 9:58 AM, Andrew Lunn wrote: >> Sorry, didn't realize SPI uses different subject format than other >> subsystems. Will fix in v3. Thanks > Each subsystem is free to use its own form. e.g for netdev you will > want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: of course they are! No one is disputing that. > > This is another reason why you should be splitting these patches per > subsystem, and submitting both the DT bindings and the code changes as > a two patch patchset. You can then learn how each subsystem names its > patches. Qualcomm QUPs chips have serial engines that can be configured as UART/I2C/SPI so QUPs changes require to be pushed in one series for all 3 subsystems as they all are dependent. > > Please pick one victim subsystem and work on the patches for just that > subsystem. Once you have them correct, you can use everything you > learned to fixup all your other patches, one by one. > > Andrew
> Qualcomm QUPs chips have serial engines that can be configured as > UART/I2C/SPI so QUPs changes require to be pushed in one series for all > 3 subsystems as they all are dependent. So leave that until later. And when you do, explicit mention why you are cross posting to three subsystems, because the hardware is designed like that. And suggest a way it could be merged, which subsystem should take the lead, and the others just need to provide Acked-by. The Maintainers might disagree, want to do it differently, but i find it always helps to state this from the beginning, otherwise sometimes no Maintainer take the lead role. But this patchset appears to be much more than QUPs. You should be able the break the rest up into smaller patchsets, one per subsystem. Andrew
On 04/09/2024 23:06, Nikunj Kela wrote: > > On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>> Sorry, didn't realize SPI uses different subject format than other >>> subsystems. Will fix in v3. Thanks >> Each subsystem is free to use its own form. e.g for netdev you will >> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: > of course they are! No one is disputing that. >> >> This is another reason why you should be splitting these patches per >> subsystem, and submitting both the DT bindings and the code changes as >> a two patch patchset. You can then learn how each subsystem names its >> patches. > > Qualcomm QUPs chips have serial engines that can be configured as > UART/I2C/SPI so QUPs changes require to be pushed in one series for all > 3 subsystems as they all are dependent. No, they are not dependent. They have never been. Look how all other upstreaming process worked in the past. Best regards, Krzysztof
On Wed, Sep 04, 2024 at 05:48:35AM GMT, Nikunj Kela wrote: > > On 9/3/2024 11:34 PM, Krzysztof Kozlowski wrote: > > On Tue, Sep 03, 2024 at 03:02:35PM -0700, Nikunj Kela wrote: > >> Add compatible representing spi support on SA8255p. > >> > >> Clocks and interconnects are being configured in firmware VM > >> on SA8255p platform, therefore making them optional. > >> > > Please use standard email subjects, so with the PATCH keyword in the > > title. helps here to create proper versioned patches. > Where did I miss PATCH keyword in the subject here? It says "[PATCH v2 > 16/21] dt-bindings: spi: document support for SA8255p" > > Another useful tool is b4. Skipping the PATCH keyword makes filtering of > > emails more difficult thus making the review process less convenient. > > > > > >> CC: Praveen Talari <quic_ptalari@quicinc.com> > >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > >> --- > >> .../bindings/spi/qcom,spi-geni-qcom.yaml | 60 +++++++++++++++++-- > >> 1 file changed, 56 insertions(+), 4 deletions(-) > >> > >> properties: > >> compatible: > >> - const: qcom,geni-spi > >> + enum: > >> + - qcom,geni-spi > >> + - qcom,sa8255p-geni-spi > > You have entire commit msg to explain why this device's programming > > model is not compatible with existing generic compatible which must > > cover all variants (because it is crazy generic). > > > > Best regards, > > Krzysztof > > I will put more details in the description of the patch, though, I had > put the description in the cover letter for this entire series. Cover letters do not land in the git repo, so the next person coming to perform modifications can not understand what was so special about this platform. Please always provide all reasoning for a change in the commit message.
On Wed, Sep 04, 2024 at 05:49:40AM GMT, Nikunj Kela wrote: > > On 9/4/2024 12:48 AM, Krzysztof Kozlowski wrote: > > On 04/09/2024 00:02, Nikunj Kela wrote: > >> Add compatible representing spi support on SA8255p. > >> > >> Clocks and interconnects are being configured in firmware VM > >> on SA8255p platform, therefore making them optional. > >> > >> CC: Praveen Talari <quic_ptalari@quicinc.com> > >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > > Also this is incomplete - adding compatible without driver change is not > > expected. It cannot even work. > > > > Best regards, > > Krzysztof > > Link for CLO branch is provided in I2C patch series. The driver changes > will soon follow. So, what's the point of posting the dt-bindings without corresponding driver changes?
On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: > On 04/09/2024 23:06, Nikunj Kela wrote: >> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>> Sorry, didn't realize SPI uses different subject format than other >>>> subsystems. Will fix in v3. Thanks >>> Each subsystem is free to use its own form. e.g for netdev you will >>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >> of course they are! No one is disputing that. >>> This is another reason why you should be splitting these patches per >>> subsystem, and submitting both the DT bindings and the code changes as >>> a two patch patchset. You can then learn how each subsystem names its >>> patches. >> Qualcomm QUPs chips have serial engines that can be configured as >> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >> 3 subsystems as they all are dependent. > No, they are not dependent. They have never been. Look how all other > upstreaming process worked in the past. Top level QUP node(patch#18) includes i2c,spi,uart nodes. soc/qcom/qcom,geni-se.yaml validate those subnodes against respective yaml. The example that is added in YAML file for QUP node will not find sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not included in the same series. > > Best regards, > Krzysztof >
On 05/09/2024 16:03, Nikunj Kela wrote: > > On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >> On 04/09/2024 23:06, Nikunj Kela wrote: >>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>> Sorry, didn't realize SPI uses different subject format than other >>>>> subsystems. Will fix in v3. Thanks >>>> Each subsystem is free to use its own form. e.g for netdev you will >>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>> of course they are! No one is disputing that. >>>> This is another reason why you should be splitting these patches per >>>> subsystem, and submitting both the DT bindings and the code changes as >>>> a two patch patchset. You can then learn how each subsystem names its >>>> patches. >>> Qualcomm QUPs chips have serial engines that can be configured as >>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>> 3 subsystems as they all are dependent. >> No, they are not dependent. They have never been. Look how all other >> upstreaming process worked in the past. > > Top level QUP node(patch#18) includes i2c,spi,uart nodes. > soc/qcom/qcom,geni-se.yaml validate those subnodes against respective > yaml. The example that is added in YAML file for QUP node will not find > sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not > included in the same series. > So where is the dependency? I don't see it. Anyway, if you insist, provide reasons why this should be the only one patchset - from all SoCs, all companies, all developers - getting an exception from standard merging practice and from explicit rule about driver change. See submitting bindings. This was re-iterated over and over, but you keep claiming you need some sort of special treatment. If so, please provide arguments WHY this requires special treatment and *all* other contributions are fine with it. Best regards, Krzysztof
On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: > On 05/09/2024 16:03, Nikunj Kela wrote: >> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>> subsystems. Will fix in v3. Thanks >>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>> of course they are! No one is disputing that. >>>>> This is another reason why you should be splitting these patches per >>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>> a two patch patchset. You can then learn how each subsystem names its >>>>> patches. >>>> Qualcomm QUPs chips have serial engines that can be configured as >>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>> 3 subsystems as they all are dependent. >>> No, they are not dependent. They have never been. Look how all other >>> upstreaming process worked in the past. >> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >> yaml. The example that is added in YAML file for QUP node will not find >> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >> included in the same series. >> > So where is the dependency? I don't see it. Ok, what is your suggestion on dt-schema check failure in that case as I mentioned above? Shall we remove examples from yaml that we added? > Anyway, if you insist, > provide reasons why this should be the only one patchset - from all > SoCs, all companies, all developers - getting an exception from standard > merging practice and from explicit rule about driver change. See > submitting bindings. > > This was re-iterated over and over, but you keep claiming you need some > sort of special treatment. If so, please provide arguments WHY this > requires special treatment and *all* other contributions are fine with it. > > Best regards, > Krzysztof >
On 05/09/2024 16:15, Nikunj Kela wrote: > > On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: >> On 05/09/2024 16:03, Nikunj Kela wrote: >>> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>>> subsystems. Will fix in v3. Thanks >>>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>>> of course they are! No one is disputing that. >>>>>> This is another reason why you should be splitting these patches per >>>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>>> a two patch patchset. You can then learn how each subsystem names its >>>>>> patches. >>>>> Qualcomm QUPs chips have serial engines that can be configured as >>>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>>> 3 subsystems as they all are dependent. >>>> No, they are not dependent. They have never been. Look how all other >>>> upstreaming process worked in the past. >>> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >>> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >>> yaml. The example that is added in YAML file for QUP node will not find >>> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >>> included in the same series. >>> >> So where is the dependency? I don't see it. > > Ok, what is your suggestion on dt-schema check failure in that case as I > mentioned above? Shall we remove examples from yaml that we added? I don't understand what sort of failure you want to fix and why examples have any problem here. I said it multiple times already but I think you never confirmed. Do you understand how patches are merged? That they go via different trees but everything must be 100% bisectable? Best regards, Krzysztof
> Ok, what is your suggestion on dt-schema check failure in that case as I > mentioned above? Shall we remove examples from yaml that we added? As Krzysztof keeps saying, Commit message. You have an unlimited amount of space to document why this SoC is special, how it is special, maybe include some ASCII art showing how it is special. Justify it being special. Once it is clear it is special, has dependencies which are real, we are likely to accept the patches. We know SoC vendors do weird things, and sometimes mainline processes just don't work. But you need to clear, upfront, and state, the process does not work because... in your commit message. Maybe put it below the ---. Something i often say to Mainline newbies. The code is easy, it is the processes which are hard. The commit message is part of the process. You want to try to anticipate all the questions Reviewers are going to ask and answer them in the commit message, before they ask them. It is process that you split patches by subsystem. It is process that binding changes and driver changes go together in the patchset. Your 'code review' should include all this, not just the lines of actual code. And to begin with, process is probably a lot more important than the actual code. So please concentrate on processes, get them right. Andrew
On 05/09/2024 16:15, Nikunj Kela wrote: > > On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: >> On 05/09/2024 16:03, Nikunj Kela wrote: >>> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>>> subsystems. Will fix in v3. Thanks >>>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>>> of course they are! No one is disputing that. >>>>>> This is another reason why you should be splitting these patches per >>>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>>> a two patch patchset. You can then learn how each subsystem names its >>>>>> patches. >>>>> Qualcomm QUPs chips have serial engines that can be configured as >>>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>>> 3 subsystems as they all are dependent. >>>> No, they are not dependent. They have never been. Look how all other >>>> upstreaming process worked in the past. >>> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >>> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >>> yaml. The example that is added in YAML file for QUP node will not find >>> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >>> included in the same series. >>> >> So where is the dependency? I don't see it. > > Ok, what is your suggestion on dt-schema check failure in that case as I > mentioned above? Shall we remove examples from yaml that we added? > > >> Anyway, if you insist, >> provide reasons why this should be the only one patchset - from all >> SoCs, all companies, all developers - getting an exception from standard >> merging practice and from explicit rule about driver change. See >> submitting bindings. >> >> This was re-iterated over and over, but you keep claiming you need some >> sort of special treatment. If so, please provide arguments WHY this >> requires special treatment and *all* other contributions are fine with it. You did not respond to above about explaining why this patchset needs special treatment, so I assume there is no exception here to be granted so any new version will follow standard process (see submitting bindings / writing bindings). Best regards, Krzysztof
On 9/5/2024 7:49 AM, Krzysztof Kozlowski wrote: > On 05/09/2024 16:15, Nikunj Kela wrote: >> On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: >>> On 05/09/2024 16:03, Nikunj Kela wrote: >>>> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>>>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>>>> subsystems. Will fix in v3. Thanks >>>>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>>>> of course they are! No one is disputing that. >>>>>>> This is another reason why you should be splitting these patches per >>>>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>>>> a two patch patchset. You can then learn how each subsystem names its >>>>>>> patches. >>>>>> Qualcomm QUPs chips have serial engines that can be configured as >>>>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>>>> 3 subsystems as they all are dependent. >>>>> No, they are not dependent. They have never been. Look how all other >>>>> upstreaming process worked in the past. >>>> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >>>> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >>>> yaml. The example that is added in YAML file for QUP node will not find >>>> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >>>> included in the same series. >>>> >>> So where is the dependency? I don't see it. >> Ok, what is your suggestion on dt-schema check failure in that case as I >> mentioned above? Shall we remove examples from yaml that we added? >> >> >>> Anyway, if you insist, >>> provide reasons why this should be the only one patchset - from all >>> SoCs, all companies, all developers - getting an exception from standard >>> merging practice and from explicit rule about driver change. See >>> submitting bindings. >>> >>> This was re-iterated over and over, but you keep claiming you need some >>> sort of special treatment. If so, please provide arguments WHY this >>> requires special treatment and *all* other contributions are fine with it. > You did not respond to above about explaining why this patchset needs > special treatment, so I assume there is no exception here to be granted > so any new version will follow standard process (see submitting bindings > / writing bindings). > > Best regards, > Krzysztof Things will be clear after you see the driver changes. Without looking at the code, this discussion won't lead to anything constructive. So I deferred the QUP related discussion until driver patches are posted. Thanks, -Nikunj >
On 9/5/2024 7:39 AM, Krzysztof Kozlowski wrote: > On 05/09/2024 16:15, Nikunj Kela wrote: >> On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: >>> On 05/09/2024 16:03, Nikunj Kela wrote: >>>> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>>>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>>>> subsystems. Will fix in v3. Thanks >>>>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>>>> of course they are! No one is disputing that. >>>>>>> This is another reason why you should be splitting these patches per >>>>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>>>> a two patch patchset. You can then learn how each subsystem names its >>>>>>> patches. >>>>>> Qualcomm QUPs chips have serial engines that can be configured as >>>>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>>>> 3 subsystems as they all are dependent. >>>>> No, they are not dependent. They have never been. Look how all other >>>>> upstreaming process worked in the past. >>>> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >>>> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >>>> yaml. The example that is added in YAML file for QUP node will not find >>>> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >>>> included in the same series. >>>> >>> So where is the dependency? I don't see it. >> Ok, what is your suggestion on dt-schema check failure in that case as I >> mentioned above? Shall we remove examples from yaml that we added? > I don't understand what sort of failure you want to fix and why examples > have any problem here. If the QUPs yaml changes are not included in the same series with i2c,serial yaml changes, you see these errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: serial@990000:compatible:0: 'qcom,sa8255p-geni-uart' is not one of ['qcom,geni-uart', 'qcom,geni-debug-uart'] /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: i2c@984000:compatible:0: 'qcom,sa8255p-geni-i2c' is not one of ['qcom,geni-i2c', 'qcom,geni-i2c-master-hub'] > I said it multiple times already but I think you > never confirmed. Do you understand how patches are merged? That they go > via different trees but everything must be 100% bisectable? > > Best regards, > Krzysztof >
> If the QUPs yaml changes are not included in the same series with > i2c,serial yaml changes, you see these errors: > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: serial@990000:compatible:0: 'qcom,sa8255p-geni-uart' is not one of ['qcom,geni-uart', 'qcom,geni-debug-uart'] > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: i2c@984000:compatible:0: 'qcom,sa8255p-geni-i2c' is not one of ['qcom,geni-i2c', 'qcom,geni-i2c-master-hub'] So you have a couple of options: 1) It sounds like you should get the QUP changes merged first. Then submit the i2c,serial changes. Is there a reason you cannot do this? Is there a mutual dependency between these two series, or just a one way dependency? 2) Explain in the commit message that following errors are expected because ... And explain in detail why the dependency cannot be broken to avoid the errors. Andrew
On 9/5/2024 9:23 AM, Andrew Lunn wrote: >> If the QUPs yaml changes are not included in the same series with >> i2c,serial yaml changes, you see these errors: >> >> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: serial@990000:compatible:0: 'qcom,sa8255p-geni-uart' is not one of ['qcom,geni-uart', 'qcom,geni-debug-uart'] >> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: i2c@984000:compatible:0: 'qcom,sa8255p-geni-i2c' is not one of ['qcom,geni-i2c', 'qcom,geni-i2c-master-hub'] > So you have a couple of options: > > 1) It sounds like you should get the QUP changes merged first. Then > submit the i2c,serial changes. Is there a reason you cannot do > this? Is there a mutual dependency between these two series, or > just a one way dependency? The ask in this thread is to create new yaml files since existing one is using generic compatibles. With new yaml, we would need to provide example and can't avoid it. If we have to provide example of QUP node, IMO, we should provide a few subnodes as well since just QUP node without subnodes(i2c/serial/spi) will not be very useful. We can possibly skip all 3 subnode and only keep one subsystem(e.g. serial) so QUP and UART yaml can go together(still need two subsystems) while SPI and I2C can go independently after QUP series is accepted. Not sure if that is acceptable to maintainers though. QUP node in actual DT will have all 3 types of subnodes(i2c,spi, serial) so example in this case won't be complete. > > 2) Explain in the commit message that following errors are expected > because ... And explain in detail why the dependency cannot be > broken to avoid the errors. > > Andrew
On 05/09/2024 18:08, Nikunj Kela wrote: > > On 9/5/2024 7:39 AM, Krzysztof Kozlowski wrote: >> On 05/09/2024 16:15, Nikunj Kela wrote: >>> On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: >>>> On 05/09/2024 16:03, Nikunj Kela wrote: >>>>> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>>>>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>>>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>>>>> subsystems. Will fix in v3. Thanks >>>>>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>>>>> of course they are! No one is disputing that. >>>>>>>> This is another reason why you should be splitting these patches per >>>>>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>>>>> a two patch patchset. You can then learn how each subsystem names its >>>>>>>> patches. >>>>>>> Qualcomm QUPs chips have serial engines that can be configured as >>>>>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>>>>> 3 subsystems as they all are dependent. >>>>>> No, they are not dependent. They have never been. Look how all other >>>>>> upstreaming process worked in the past. >>>>> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >>>>> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >>>>> yaml. The example that is added in YAML file for QUP node will not find >>>>> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >>>>> included in the same series. >>>>> >>>> So where is the dependency? I don't see it. >>> Ok, what is your suggestion on dt-schema check failure in that case as I >>> mentioned above? Shall we remove examples from yaml that we added? >> I don't understand what sort of failure you want to fix and why examples >> have any problem here. > > If the QUPs yaml changes are not included in the same series with They cannot be included in the same series. You just think that including here solves the problem so go ahead, simulate the merging: 1. Bjorn applies soc/qcom/qcom,geni-se.yaml patch and tests. His tree MUST build, so it also must pass dt_binding_check. Does it pass? No. 2. SPI maintainer... ah, no point even going there. > i2c,serial yaml changes, you see these errors: > > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: serial@990000:compatible:0: 'qcom,sa8255p-geni-uart' is not one of ['qcom,geni-uart', 'qcom,geni-debug-uart'] > /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: i2c@984000:compatible:0: 'qcom,sa8255p-geni-i2c' is not one of ['qcom,geni-i2c', 'qcom,geni-i2c-master-hub'] Don't grow examples if not needed. Or create dependencies and ask maintainers to cross-merge. Best regards, Krzysztof
On 05/09/2024 18:56, Krzysztof Kozlowski wrote: > On 05/09/2024 18:08, Nikunj Kela wrote: >> >> On 9/5/2024 7:39 AM, Krzysztof Kozlowski wrote: >>> On 05/09/2024 16:15, Nikunj Kela wrote: >>>> On 9/5/2024 7:09 AM, Krzysztof Kozlowski wrote: >>>>> On 05/09/2024 16:03, Nikunj Kela wrote: >>>>>> On 9/5/2024 1:04 AM, Krzysztof Kozlowski wrote: >>>>>>> On 04/09/2024 23:06, Nikunj Kela wrote: >>>>>>>> On 9/4/2024 9:58 AM, Andrew Lunn wrote: >>>>>>>>>> Sorry, didn't realize SPI uses different subject format than other >>>>>>>>>> subsystems. Will fix in v3. Thanks >>>>>>>>> Each subsystem is free to use its own form. e.g for netdev you will >>>>>>>>> want the prefix [PATCH net-next v42] net: stmmac: dwmac-qcom-ethqos: >>>>>>>> of course they are! No one is disputing that. >>>>>>>>> This is another reason why you should be splitting these patches per >>>>>>>>> subsystem, and submitting both the DT bindings and the code changes as >>>>>>>>> a two patch patchset. You can then learn how each subsystem names its >>>>>>>>> patches. >>>>>>>> Qualcomm QUPs chips have serial engines that can be configured as >>>>>>>> UART/I2C/SPI so QUPs changes require to be pushed in one series for all >>>>>>>> 3 subsystems as they all are dependent. >>>>>>> No, they are not dependent. They have never been. Look how all other >>>>>>> upstreaming process worked in the past. >>>>>> Top level QUP node(patch#18) includes i2c,spi,uart nodes. >>>>>> soc/qcom/qcom,geni-se.yaml validate those subnodes against respective >>>>>> yaml. The example that is added in YAML file for QUP node will not find >>>>>> sa8255p compatibles if all 4 yaml(qup, i2c, spi, serial nodes) are not >>>>>> included in the same series. >>>>>> >>>>> So where is the dependency? I don't see it. >>>> Ok, what is your suggestion on dt-schema check failure in that case as I >>>> mentioned above? Shall we remove examples from yaml that we added? >>> I don't understand what sort of failure you want to fix and why examples >>> have any problem here. >> >> If the QUPs yaml changes are not included in the same series with > > They cannot be included in the same series. You just think that > including here solves the problem so go ahead, simulate the merging: > 1. Bjorn applies soc/qcom/qcom,geni-se.yaml patch and tests. His tree > MUST build, so it also must pass dt_binding_check. > Does it pass? No. > > 2. SPI maintainer... ah, no point even going there. > >> i2c,serial yaml changes, you see these errors: >> >> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: serial@990000:compatible:0: 'qcom,sa8255p-geni-uart' is not one of ['qcom,geni-uart', 'qcom,geni-debug-uart'] >> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: i2c@984000:compatible:0: 'qcom,sa8255p-geni-i2c' is not one of ['qcom,geni-i2c', 'qcom,geni-i2c-master-hub'] > > Don't grow examples if not needed. Or create dependencies and ask > maintainers to cross-merge. Or soc/geni-se binding could be also converted to just list compatibles instead of referencing other schema, just like MDSS. Best regards, Krzysztof
On Thu, Sep 05, 2024 at 09:39:54AM -0700, Nikunj Kela wrote: > > On 9/5/2024 9:23 AM, Andrew Lunn wrote: > >> If the QUPs yaml changes are not included in the same series with > >> i2c,serial yaml changes, you see these errors: > >> > >> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: serial@990000:compatible:0: 'qcom,sa8255p-geni-uart' is not one of ['qcom,geni-uart', 'qcom,geni-debug-uart'] > >> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.example.dtb: geniqup@9c0000: i2c@984000:compatible:0: 'qcom,sa8255p-geni-i2c' is not one of ['qcom,geni-i2c', 'qcom,geni-i2c-master-hub'] > > So you have a couple of options: > > > > 1) It sounds like you should get the QUP changes merged first. Then > > submit the i2c,serial changes. Is there a reason you cannot do > > this? Is there a mutual dependency between these two series, or > > just a one way dependency? > > The ask in this thread is to create new yaml files since existing one is > using generic compatibles. With new yaml, we would need to provide > example and can't avoid it. If we have to provide example of QUP node, > IMO, we should provide a few subnodes as well since just QUP node > without subnodes(i2c/serial/spi) will not be very useful. Does it need to be useful, at the beginning? Was the development done all at once, i2c, serial and spi all mixed together, inseparable? More likely, you have a set of patches adding some sort of base, and hopefully a DT binding patch for that base. Then you add a driver in drivers/tty/serial, with patches which extend the DT binding with the serial port. You then add a driver in driver/i2c/busses and extend the DT binding for I2C. And then add a driver for SPI in drivers/spi, which again extends the DT binding? This would be typical for how an MFD would be posted. Please go search the lists for examples of MFDs you might be able to follow. Andrew
On 9/4/2024 6:21 AM, Krzysztof Kozlowski wrote: > On 04/09/2024 14:48, Nikunj Kela wrote: >> On 9/3/2024 11:34 PM, Krzysztof Kozlowski wrote: >>> On Tue, Sep 03, 2024 at 03:02:35PM -0700, Nikunj Kela wrote: >>>> Add compatible representing spi support on SA8255p. >>>> >>>> Clocks and interconnects are being configured in firmware VM >>>> on SA8255p platform, therefore making them optional. >>>> >>> Please use standard email subjects, so with the PATCH keyword in the >>> title. helps here to create proper versioned patches. >> Where did I miss PATCH keyword in the subject here? It says "[PATCH v2 >> 16/21] dt-bindings: spi: document support for SA8255p" > Oh, wrong template. It was about spi prefix, These are the latest 4 commits in linux-next for spi: 12736adc43b7 dt-bindings: spi: nxp-fspi: add imx8ulp support b0cdf9cc0895 spi: dt-bindings: Add rockchip,rk3576-spi compatible d6d0af1b9eff dt-bindings: spi: add PIC64GX SPI/QSPI compatibility to MPFS SPI/QSPI bindings 1c4d834e4e81 spi: dt-bindings: convert spi-sc18is602.txt to yaml format Now I am confused which prefix format shall I use? first spi or first dt-bindings? > should be this one: > > Please use subject prefixes matching the subsystem. You can get them for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching. For bindings, the preferred subjects are > explained here: > https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters > > Best regards, > Krzysztof >
On Mon, Sep 09, 2024 at 01:29:37PM -0700, Nikunj Kela wrote: > Now I am confused which prefix format shall I use? first spi or first > dt-bindings? spi: first.
diff --git a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml index 2e20ca313ec1..75b52c0a7440 100644 --- a/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml +++ b/Documentation/devicetree/bindings/spi/qcom,spi-geni-qcom.yaml @@ -25,10 +25,45 @@ description: allOf: - $ref: /schemas/spi/spi-controller.yaml# + - if: + properties: + compatible: + contains: + const: qcom,sa8255p-geni-spi + then: + required: + - power-domains + - power-domain-names + + properties: + power-domains: + minItems: 2 + + else: + required: + - clocks + - clock-names + + properties: + power-domains: + maxItems: 1 + + interconnects: + minItems: 2 + maxItems: 3 + + interconnect-names: + minItems: 2 + items: + - const: qup-core + - const: qup-config + - const: qup-memory properties: compatible: - const: qcom,geni-spi + enum: + - qcom,geni-spi + - qcom,sa8255p-geni-spi clocks: maxItems: 1 @@ -61,15 +96,19 @@ properties: operating-points-v2: true power-domains: - maxItems: 1 + minItems: 1 + maxItems: 2 + + power-domain-names: + items: + - const: power + - const: perf reg: maxItems: 1 required: - compatible - - clocks - - clock-names - interrupts - reg @@ -116,3 +155,16 @@ examples: #address-cells = <1>; #size-cells = <0>; }; + + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + spi@888000 { + compatible = "qcom,sa8255p-geni-spi"; + reg = <0x888000 0x4000>; + interrupts = <GIC_SPI 584 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + power-domains = <&scmi10_pd 16>, <&scmi10_dvfs 16>; + power-domain-names = "power", "perf"; + };
Add compatible representing spi support on SA8255p. Clocks and interconnects are being configured in firmware VM on SA8255p platform, therefore making them optional. CC: Praveen Talari <quic_ptalari@quicinc.com> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> --- .../bindings/spi/qcom,spi-geni-qcom.yaml | 60 +++++++++++++++++-- 1 file changed, 56 insertions(+), 4 deletions(-)