| Message ID | 20240903220240.2594102-16-quic_nkela@quicinc.com |
|---|---|
| State | New |
| 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:34PM -0700, Nikunj Kela wrote: > Add compatible representing i2c support on SA8255p. > > Clocks and interconnects are being configured in Firmware VM > on SA8255p, therefore making them optional. > > CC: Praveen Talari <quic_ptalari@quicinc.com> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> > --- > .../bindings/i2c/qcom,i2c-geni-qcom.yaml | 33 +++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > I don't know what to do with this patch. Using specific compatibles next to generic compatible is just wrong, although mistake was probably allowing generic compatible. The patch does not explain the differences in interface which would explain why devices are not compatible. In the same time my advice of separate binding was not followed, because maybe these devices are compatible? But then it should be expressed... You have entire commit msg to explain what and why. > diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > index 9f66a3bb1f80..b477fae734b6 100644 > --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml > @@ -15,6 +15,7 @@ properties: > enum: > - qcom,geni-i2c > - qcom,geni-i2c-master-hub > + - qcom,sa8255p-geni-i2c > > clocks: > minItems: 1 > @@ -69,8 +70,6 @@ properties: > required: > - compatible > - interrupts > - - clocks > - - clock-names > - reg > > allOf: > @@ -81,6 +80,10 @@ allOf: > contains: > const: qcom,geni-i2c-master-hub > then: > + required: > + - clocks > + - clock-names So it is required here? > + > properties: > clocks: > minItems: 2 > @@ -100,7 +103,21 @@ allOf: > items: > - const: qup-core > - const: qup-config > + > + - if: > + properties: > + compatible: > + contains: > + const: qcom,sa8255p-geni-i2c > + then: > + required: > + - power-domains > + And possible here? I assume with the same clocks? The same for interconnects - same values are valid? > else: > + required: > + - clocks > + - clock-names And clocks are required again? > + > properties: > clocks: > maxItems: 1 Eeee? So now all other variants have max 1 clock? Nope, this wasn't ever tested on real DTS. Best regards, Krzysztof
On 9/3/2024 11:31 PM, Krzysztof Kozlowski wrote: > On Tue, Sep 03, 2024 at 03:02:34PM -0700, Nikunj Kela wrote: >> Add compatible representing i2c support on SA8255p. >> >> Clocks and interconnects are being configured in Firmware VM >> on SA8255p, therefore making them optional. >> >> CC: Praveen Talari <quic_ptalari@quicinc.com> >> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> >> --- >> .../bindings/i2c/qcom,i2c-geni-qcom.yaml | 33 +++++++++++++++++-- >> 1 file changed, 31 insertions(+), 2 deletions(-) >> > I don't know what to do with this patch. Using specific compatibles next > to generic compatible is just wrong, although mistake was probably > allowing generic compatible. The patch does not explain the differences > in interface which would explain why devices are not compatible. I mentioned in the description that clocks and interconnects on this platform are configured in Firmware VM(over SCMI using power and perf domains) therefore this is not compatible with existing generic compatible. > In the > same time my advice of separate binding was not followed, because maybe > these devices are compatible? But then it should be expressed... Sorry, I missed that. You want me to use 'oneOf' expression with this compatible? > > You have entire commit msg to explain what and why. Will put more details in description. >> diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> index 9f66a3bb1f80..b477fae734b6 100644 >> --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml >> @@ -15,6 +15,7 @@ properties: >> enum: >> - qcom,geni-i2c >> - qcom,geni-i2c-master-hub >> + - qcom,sa8255p-geni-i2c >> >> clocks: >> minItems: 1 >> @@ -69,8 +70,6 @@ properties: >> required: >> - compatible >> - interrupts >> - - clocks >> - - clock-names >> - reg >> >> allOf: >> @@ -81,6 +80,10 @@ allOf: >> contains: >> const: qcom,geni-i2c-master-hub >> then: >> + required: >> + - clocks >> + - clock-names > > So it is required here? We are removing clocks from generic required list and enforcing rules for all compatibles other than sa8255p. >> + >> properties: >> clocks: >> minItems: 2 >> @@ -100,7 +103,21 @@ allOf: >> items: >> - const: qup-core >> - const: qup-config >> + >> + - if: >> + properties: >> + compatible: >> + contains: >> + const: qcom,sa8255p-geni-i2c >> + then: >> + required: >> + - power-domains >> + > And possible here? I assume with the same clocks? The same for > interconnects - same values are valid? I guess I need to put here the same description as in the cover letter to make it more clear. We are not using clocks and interconnects in this platform in Linux. Instead, sending request to Firmware VM over SCMI(using power and perf protocols) > >> else: >> + required: >> + - clocks >> + - clock-names > And clocks are required again? Explained above. >> + >> properties: >> clocks: >> maxItems: 1 > Eeee? So now all other variants have max 1 clock? I will make if block for sa8255p up so else is not applied to rest of the platforms. > > Nope, this wasn't ever tested on real DTS. This is tested on SA8255p DTS and I ran DT schema check on SA8775p DT as well. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml index 9f66a3bb1f80..b477fae734b6 100644 --- a/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml +++ b/Documentation/devicetree/bindings/i2c/qcom,i2c-geni-qcom.yaml @@ -15,6 +15,7 @@ properties: enum: - qcom,geni-i2c - qcom,geni-i2c-master-hub + - qcom,sa8255p-geni-i2c clocks: minItems: 1 @@ -69,8 +70,6 @@ properties: required: - compatible - interrupts - - clocks - - clock-names - reg allOf: @@ -81,6 +80,10 @@ allOf: contains: const: qcom,geni-i2c-master-hub then: + required: + - clocks + - clock-names + properties: clocks: minItems: 2 @@ -100,7 +103,21 @@ allOf: items: - const: qup-core - const: qup-config + + - if: + properties: + compatible: + contains: + const: qcom,sa8255p-geni-i2c + then: + required: + - power-domains + else: + required: + - clocks + - clock-names + properties: clocks: maxItems: 1 @@ -143,4 +160,16 @@ examples: power-domains = <&rpmhpd SC7180_CX>; required-opps = <&rpmhpd_opp_low_svs>; }; + + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + + i2c@a90000 { + compatible = "qcom,sa8255p-geni-i2c"; + reg = <0xa90000 0x4000>; + interrupts = <GIC_SPI 357 IRQ_TYPE_LEVEL_HIGH>; + #address-cells = <1>; + #size-cells = <0>; + power-domains = <&scmi9_pd 11>; + }; ...
Add compatible representing i2c support on SA8255p. Clocks and interconnects are being configured in Firmware VM on SA8255p, therefore making them optional. CC: Praveen Talari <quic_ptalari@quicinc.com> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> --- .../bindings/i2c/qcom,i2c-geni-qcom.yaml | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)