| Message ID | 20240903220240.2594102-16-quic_nkela@quicinc.com (mailing list archive) |
|---|---|
| State | New, 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: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 04/09/2024 00:02, 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(-) > Just to clarify to I2C maintainers: This is incomplete. Missing driver changes. Best regards, Krzysztof
> Just to clarify to I2C maintainers: > This is incomplete. Missing driver changes. Thanks, 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 >
On 9/4/2024 12:55 AM, Wolfram Sang wrote: >> Just to clarify to I2C maintainers: >> This is incomplete. Missing driver changes. > Thanks, Krzysztof! Driver changes are going through internal review and will soon be posted. For your reference, we have pushed driver changes in CodeLinaro git branch(nkela/sa8255p_v6_11_rc2) in kernel-qcom repo [1]. You can take a look at the changes that are in pipeline and will follow soon. [1]: https://git.codelinaro.org/clo/linux-kernel/kernel-qcom/-/tree/nkela/sa8255p_v6_11_rc2?ref_type=heads
On 04/09/2024 14:41, Nikunj Kela wrote: > > 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. It is not obvious to me. I doubt it is obvious to others. Commit msg does not say they are compatible and usually difference in clocks/interconnects is not reason of incompatibility. So why suddenly here we would understand it differently? > > >> 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? I proposed separate binding file. But your commit msg suggested these are compatible. Lack of driver change is also proof of that. I don't want to keep discussing this because it does not lead to anywhere. We keep repeating the same. > > >> >> 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. You just affected all the DTS everywhere. It's your task to check all DTS everywhere. Not ours. Best regards, Krzysztof
On 04/09/2024 14:45, Nikunj Kela wrote: > > On 9/4/2024 12:55 AM, Wolfram Sang wrote: >>> Just to clarify to I2C maintainers: >>> This is incomplete. Missing driver changes. >> Thanks, Krzysztof! > > Driver changes are going through internal review and will soon be > posted. For your reference, we have pushed driver changes in CodeLinaro > git branch(nkela/sa8255p_v6_11_rc2) in kernel-qcom repo [1]. You can > take a look at the changes that are in pipeline and will follow soon. > Sorry, we are not reviewing other repos. Post patches ONLY when they are ready. Sending one piece without driver is not correct and it does not make any, absolutely any sense. Best regards, Krzysztof
Hi Nikunj, On Wed, Sep 04, 2024 at 05:45:05AM GMT, Nikunj Kela wrote: > > On 9/4/2024 12:55 AM, Wolfram Sang wrote: > >> Just to clarify to I2C maintainers: > >> This is incomplete. Missing driver changes. > > Thanks, Krzysztof! > > Driver changes are going through internal review and will soon be > posted. For your reference, we have pushed driver changes in CodeLinaro > git branch(nkela/sa8255p_v6_11_rc2) in kernel-qcom repo [1]. You can > take a look at the changes that are in pipeline and will follow soon. Please post here driver changes along with the DTS updates. Thanks Krzysztof for being active here! Andi
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(-)