| Message ID | 20240903220240.2594102-18-quic_nkela@quicinc.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| 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:36PM -0700, Nikunj Kela wrote: > Add compatibles representing UART support on SA8255p. > > Clocks and interconnects are being configured in the 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> > --- > .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- > 1 file changed, 47 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml > index dd33794b3534..b63c984684f3 100644 > --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml > +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml > @@ -10,14 +10,13 @@ maintainers: > - Andy Gross <agross@kernel.org> > - Bjorn Andersson <bjorn.andersson@linaro.org> > > -allOf: > - - $ref: /schemas/serial/serial.yaml# > - > properties: > compatible: > enum: > - qcom,geni-uart > - qcom,geni-debug-uart > + - qcom,sa8255p-geni-uart > + - qcom,sa8255p-geni-debug-uart Why devices are not compatible? What changed in programming model? > > clocks: > maxItems: 1 > @@ -51,18 +50,49 @@ properties: > - const: sleep > > power-domains: > - maxItems: 1 > + minItems: 1 > + maxItems: 2 > + > + power-domain-names: This does not match power-domains anymore. > + items: > + - const: power > + - const: perf > > reg: > maxItems: 1 > > required: > - compatible > - - clocks > - - clock-names > - interrupts > - reg > > +allOf: > + - $ref: /schemas/serial/serial.yaml# > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,sa8255p-geni-uart > + - qcom,sa8255p-geni-debug-uart > + then: > + required: > + - power-domains > + - power-domain-names > + > + properties: > + power-domains: > + minItems: 2 > + > + else: > + required: > + - clocks > + - clock-names > + > + properties: > + power-domains: > + maxItems: 1 > + > unevaluatedProperties: false > > examples: > @@ -83,4 +113,15 @@ examples: > <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>; > interconnect-names = "qup-core", "qup-config"; > }; > + > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + serial@990000 { > + compatible = "qcom,sa8255p-geni-uart"; > + reg = <0x990000 0x4000>; > + interrupts = <GIC_SPI 531 IRQ_TYPE_LEVEL_HIGH>; > + power-domains = <&scmi11_pd 4>, <&scmi11_dvfs 4>; > + power-domain-names = "power", "perf"; > + }; > ... > -- > 2.34.1 >
On 04/09/2024 00:02, Nikunj Kela wrote: > Add compatibles representing UART support on SA8255p. > > Clocks and interconnects are being configured in the 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> > --- > .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- > 1 file changed, 47 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml > index dd33794b3534..b63c984684f3 100644 > --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml > +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml > @@ -10,14 +10,13 @@ maintainers: > - Andy Gross <agross@kernel.org> > - Bjorn Andersson <bjorn.andersson@linaro.org> > > -allOf: > - - $ref: /schemas/serial/serial.yaml# > - > properties: > compatible: > enum: > - qcom,geni-uart > - qcom,geni-debug-uart > + - qcom,sa8255p-geni-uart > + - qcom,sa8255p-geni-debug-uart Anyway, the entire patchset is organized wrong. Or you sent only subset. Where is the driver change? This cannot work. To remind bindings go with the driver (nothing new here). Best regards, Krzysztof
On 9/3/2024 11:36 PM, Krzysztof Kozlowski wrote: > On Tue, Sep 03, 2024 at 03:02:36PM -0700, Nikunj Kela wrote: >> Add compatibles representing UART support on SA8255p. >> >> Clocks and interconnects are being configured in the 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> >> --- >> .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- >> 1 file changed, 47 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >> index dd33794b3534..b63c984684f3 100644 >> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >> @@ -10,14 +10,13 @@ maintainers: >> - Andy Gross <agross@kernel.org> >> - Bjorn Andersson <bjorn.andersson@linaro.org> >> >> -allOf: >> - - $ref: /schemas/serial/serial.yaml# >> - >> properties: >> compatible: >> enum: >> - qcom,geni-uart >> - qcom,geni-debug-uart >> + - qcom,sa8255p-geni-uart >> + - qcom,sa8255p-geni-debug-uart > Why devices are not compatible? What changed in programming model? The cover-letter explains what is changed for devices in this platform. I will add the description in this patch too. > >> >> clocks: >> maxItems: 1 >> @@ -51,18 +50,49 @@ properties: >> - const: sleep >> >> power-domains: >> - maxItems: 1 >> + minItems: 1 >> + maxItems: 2 >> + >> + power-domain-names: > This does not match power-domains anymore. Single power domain doesn't need to use power-domain-names binding as it is not needed however for multiple(in this case 2), you need to provide names. I will add this property to if block and only keep maxItems here. > >> + items: >> + - const: power >> + - const: perf >> >> reg: >> maxItems: 1 >> >> required: >> - compatible >> - - clocks >> - - clock-names >> - interrupts >> - reg >> >> +allOf: >> + - $ref: /schemas/serial/serial.yaml# >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - qcom,sa8255p-geni-uart >> + - qcom,sa8255p-geni-debug-uart >> + then: >> + required: >> + - power-domains >> + - power-domain-names >> + >> + properties: >> + power-domains: >> + minItems: 2 >> + >> + else: >> + required: >> + - clocks >> + - clock-names >> + >> + properties: >> + power-domains: >> + maxItems: 1 >> + >> unevaluatedProperties: false >> >> examples: >> @@ -83,4 +113,15 @@ examples: >> <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>; >> interconnect-names = "qup-core", "qup-config"; >> }; >> + >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> + serial@990000 { >> + compatible = "qcom,sa8255p-geni-uart"; >> + reg = <0x990000 0x4000>; >> + interrupts = <GIC_SPI 531 IRQ_TYPE_LEVEL_HIGH>; >> + power-domains = <&scmi11_pd 4>, <&scmi11_dvfs 4>; >> + power-domain-names = "power", "perf"; >> + }; >> ... >> -- >> 2.34.1 >>
On 9/4/2024 12:47 AM, Krzysztof Kozlowski wrote: > On 04/09/2024 00:02, Nikunj Kela wrote: >> Add compatibles representing UART support on SA8255p. >> >> Clocks and interconnects are being configured in the 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> >> --- >> .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- >> 1 file changed, 47 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >> index dd33794b3534..b63c984684f3 100644 >> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >> @@ -10,14 +10,13 @@ maintainers: >> - Andy Gross <agross@kernel.org> >> - Bjorn Andersson <bjorn.andersson@linaro.org> >> >> -allOf: >> - - $ref: /schemas/serial/serial.yaml# >> - >> properties: >> compatible: >> enum: >> - qcom,geni-uart >> - qcom,geni-debug-uart >> + - qcom,sa8255p-geni-uart >> + - qcom,sa8255p-geni-debug-uart > > Anyway, the entire patchset is organized wrong. Or you sent only subset. > > Where is the driver change? This cannot work. To remind bindings go with > the driver (nothing new here). > > Best regards, > Krzysztof The driver changes will soon be posted. They are being reviewed internally. For a quick look on what is coming next, you can refer to CodeLinaro git repo[1] [1]: https://git.codelinaro.org/clo/linux-kernel/kernel-qcom/-/tree/nkela/sa8255p_v6_11_rc2?ref_type=heads
On 04/09/2024 14:56, Nikunj Kela wrote: > > On 9/4/2024 12:47 AM, Krzysztof Kozlowski wrote: >> On 04/09/2024 00:02, Nikunj Kela wrote: >>> Add compatibles representing UART support on SA8255p. >>> >>> Clocks and interconnects are being configured in the 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> >>> --- >>> .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- >>> 1 file changed, 47 insertions(+), 6 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >>> index dd33794b3534..b63c984684f3 100644 >>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >>> @@ -10,14 +10,13 @@ maintainers: >>> - Andy Gross <agross@kernel.org> >>> - Bjorn Andersson <bjorn.andersson@linaro.org> >>> >>> -allOf: >>> - - $ref: /schemas/serial/serial.yaml# >>> - >>> properties: >>> compatible: >>> enum: >>> - qcom,geni-uart >>> - qcom,geni-debug-uart >>> + - qcom,sa8255p-geni-uart >>> + - qcom,sa8255p-geni-debug-uart >> >> Anyway, the entire patchset is organized wrong. Or you sent only subset. >> >> Where is the driver change? This cannot work. To remind bindings go with >> the driver (nothing new here). >> >> Best regards, >> Krzysztof > > The driver changes will soon be posted. They are being reviewed > internally. For a quick look on what is coming next, you can refer to > CodeLinaro git repo[1] Upstream does not work like that. This patch is just wrong and pointless without driver change. Never send such stuff separately from the driver. Or fix the binding, if the intention was there is no driver. Best regards, Krzysztof
On 04/09/2024 14:54, Nikunj Kela wrote: > > On 9/3/2024 11:36 PM, Krzysztof Kozlowski wrote: >> On Tue, Sep 03, 2024 at 03:02:36PM -0700, Nikunj Kela wrote: >>> Add compatibles representing UART support on SA8255p. >>> >>> Clocks and interconnects are being configured in the 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> >>> --- >>> .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- >>> 1 file changed, 47 insertions(+), 6 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >>> index dd33794b3534..b63c984684f3 100644 >>> --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >>> +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml >>> @@ -10,14 +10,13 @@ maintainers: >>> - Andy Gross <agross@kernel.org> >>> - Bjorn Andersson <bjorn.andersson@linaro.org> >>> >>> -allOf: >>> - - $ref: /schemas/serial/serial.yaml# >>> - >>> properties: >>> compatible: >>> enum: >>> - qcom,geni-uart >>> - qcom,geni-debug-uart >>> + - qcom,sa8255p-geni-uart >>> + - qcom,sa8255p-geni-debug-uart >> Why devices are not compatible? What changed in programming model? > > The cover-letter explains what is changed for devices in this platform. > I will add the description in this patch too. Many of us do not read cover letters. They don't really matter, especially that serial tree will not include it. Each commit must stand on its own. > > >> >>> >>> clocks: >>> maxItems: 1 >>> @@ -51,18 +50,49 @@ properties: >>> - const: sleep >>> >>> power-domains: >>> - maxItems: 1 >>> + minItems: 1 >>> + maxItems: 2 >>> + >>> + power-domain-names: >> This does not match power-domains anymore. > > Single power domain doesn't need to use power-domain-names binding as it > is not needed however for multiple(in this case 2), you need to provide > names. I will add this property to if block and only keep maxItems here. The xxx and xxx-names properties always go in sync. Otherwise we do not really know what is the power domain for other variants. You are allowed to be unspecific about power domain (so maxItems: 1) if it is obvious. You now made it non-obvious, so above flexibility does not apply anymore. Best regards, Krzysztof
> The driver changes will soon be posted. They are being reviewed > internally. And what do you do when internal reviewers tell you that everything is wrong and you need to change the binding? You just wasted a lot of peoples time. Please don't post patches until you know they are correct, complete, build W=1, and pass all the standard static analysers. I suggest you try to find an experience Mainline developer who can mentor you. Andrew
On 9/4/2024 10:05 AM, Andrew Lunn wrote: >> The driver changes will soon be posted. They are being reviewed >> internally. > And what do you do when internal reviewers tell you that everything is > wrong and you need to change the binding? You just wasted a lot of > peoples time. Let me clarify here, the patches have already been through multiple rounds of review and since this is new architecture that we are using, we want to make sure this gets reviewed internally as much as possible. While, we will be posting them soon, they are available on public git repo for anyone to take a feel of the amount of changes. Let's not be judgemental here. > Please don't post patches until you know they are correct, complete, > build W=1, and pass all the standard static analysers. > > I suggest you try to find an experience Mainline developer who can > mentor you. No one is born with experience. You learn as you go. Please note that this series has gone through internal review before I posted it in upstream. > Andrew
> No one is born with experience. You learn as you go. Please note that > this series has gone through internal review before I posted it in > upstream. Then i'm surprise you were not told to submit lots of smaller patchsets, one per subsystem, which are complete. I get nobody is born with experience, but for a company the size of Qualcomm, they can easily hire a few experienced mainline developers who can mentor you, rather than having overloaded Maintainers teach you the basics, and getting frustrated in the process. Andrew
On 04/09/2024 23:54, Andrew Lunn wrote: >> No one is born with experience. You learn as you go. Please note that >> this series has gone through internal review before I posted it in >> upstream. > > Then i'm surprise you were not told to submit lots of smaller > patchsets, one per subsystem, which are complete. We did... multiple times. We gave examples how entire new Qualcomm SoC should be upstreamed, how this process should be organized. We gave trainings. Some listen, some not. Sometimes people do not even come to a training (for free). But there will be always an excuse for patchset doing something entirely different than community expects... The patchset here is a result of some misconceptions and not understanding what is the dependency (claiming there is while there is no) and what are the maintainer trees. > > I get nobody is born with experience, but for a company the size of > Qualcomm, they can easily hire a few experienced mainline developers > who can mentor you, rather than having overloaded Maintainers teach > you the basics, and getting frustrated in the process. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml index dd33794b3534..b63c984684f3 100644 --- a/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml +++ b/Documentation/devicetree/bindings/serial/qcom,serial-geni-qcom.yaml @@ -10,14 +10,13 @@ maintainers: - Andy Gross <agross@kernel.org> - Bjorn Andersson <bjorn.andersson@linaro.org> -allOf: - - $ref: /schemas/serial/serial.yaml# - properties: compatible: enum: - qcom,geni-uart - qcom,geni-debug-uart + - qcom,sa8255p-geni-uart + - qcom,sa8255p-geni-debug-uart clocks: maxItems: 1 @@ -51,18 +50,49 @@ properties: - const: sleep 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 +allOf: + - $ref: /schemas/serial/serial.yaml# + - if: + properties: + compatible: + contains: + enum: + - qcom,sa8255p-geni-uart + - qcom,sa8255p-geni-debug-uart + then: + required: + - power-domains + - power-domain-names + + properties: + power-domains: + minItems: 2 + + else: + required: + - clocks + - clock-names + + properties: + power-domains: + maxItems: 1 + unevaluatedProperties: false examples: @@ -83,4 +113,15 @@ examples: <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_QUP_0 0>; interconnect-names = "qup-core", "qup-config"; }; + + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + serial@990000 { + compatible = "qcom,sa8255p-geni-uart"; + reg = <0x990000 0x4000>; + interrupts = <GIC_SPI 531 IRQ_TYPE_LEVEL_HIGH>; + power-domains = <&scmi11_pd 4>, <&scmi11_dvfs 4>; + power-domain-names = "power", "perf"; + }; ...
Add compatibles representing UART support on SA8255p. Clocks and interconnects are being configured in the 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> --- .../serial/qcom,serial-geni-qcom.yaml | 53 ++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-)