| Message ID | 20240117102526.557006-2-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Fix and update ti,j721e-pci-* bindings | expand |
On 17/01/2024 11:25, Siddharth Vadapalli wrote: > The existing implementation for validating the "num-lanes" property > based on the compatible(s) doesn't enforce it. Fix it by updating the > checks to handle both single-compatible and multi-compatible cases. > > Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes") > Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings") > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > .../bindings/pci/ti,j721e-pci-ep.yaml | 26 ++++++++++++++----- > .../bindings/pci/ti,j721e-pci-host.yaml | 26 ++++++++++++++----- > 2 files changed, 38 insertions(+), 14 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml > index 97f2579ea908..278e0892f8ac 100644 > --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml > +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml > @@ -68,8 +68,9 @@ allOf: > - if: > properties: > compatible: Missing contains:, instead of your change. > - enum: > - - ti,am64-pcie-ep > + items: > + - const: ti,am64-pcie-ep > + - const: ti,j721e-pcie-ep > then: > properties: > num-lanes: > @@ -78,9 +79,9 @@ allOf: > - if: > properties: > compatible: > - enum: > - - ti,j7200-pcie-ep > - - ti,j721e-pcie-ep > + items: > + - const: ti,j7200-pcie-ep > + - const: ti,j721e-pcie-ep "Ditto > then: > properties: > num-lanes: > @@ -90,8 +91,19 @@ allOf: > - if: > properties: > compatible: > - enum: > - - ti,j784s4-pcie-ep > + items: > + - const: ti,j721e-pcie-ep > + then: > + properties: > + num-lanes: > + minimum: 1 > + maximum: 4 > + > + - if: > + properties: > + compatible: > + items: > + - const: ti,j784s4-pcie-ep Why? Previous code was correct. Best regards, Krzysztof
Hello Krzysztof, On 17/01/24 16:04, Krzysztof Kozlowski wrote: > On 17/01/2024 11:25, Siddharth Vadapalli wrote: >> The existing implementation for validating the "num-lanes" property >> based on the compatible(s) doesn't enforce it. Fix it by updating the >> checks to handle both single-compatible and multi-compatible cases. >> >> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes") >> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings") >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> .../bindings/pci/ti,j721e-pci-ep.yaml | 26 ++++++++++++++----- >> .../bindings/pci/ti,j721e-pci-host.yaml | 26 ++++++++++++++----- >> 2 files changed, 38 insertions(+), 14 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >> index 97f2579ea908..278e0892f8ac 100644 >> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >> @@ -68,8 +68,9 @@ allOf: >> - if: >> properties: >> compatible: > > Missing contains:, instead of your change. I did try the "contains" approach before determining that the implementation in this patch is more suitable. Please consider the following: For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep". Therefore, the device-tree nodes for AM64 and J7200 look like: AM64: compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep"; ... num-lanes = 1; J7200: compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep"; ... num-lanes = 4; This implies that when the check for "num-lanes" is performed on the device-tree node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the AM64's "compatible: contains:" check will match the schema and it will check the existing "num-lanes" being described as "const: 1" against the value in J7200's PCIe node resulting in a warning. Therefore, using "contains" will result in errors if the check has to be performed for device-tree nodes with fallback compatibles. The "items" based approach I have used in this patch ensures that the schema matches *only* when both the primary and fallback compatible are present in the device-tree node. > >> - enum: >> - - ti,am64-pcie-ep >> + items: >> + - const: ti,am64-pcie-ep >> + - const: ti,j721e-pcie-ep > >> then: >> properties: >> num-lanes: >> @@ -78,9 +79,9 @@ allOf: >> - if: >> properties: >> compatible: >> - enum: >> - - ti,j7200-pcie-ep >> - - ti,j721e-pcie-ep >> + items: >> + - const: ti,j7200-pcie-ep >> + - const: ti,j721e-pcie-ep > > "Ditto Same explanation as above. > >> then: >> properties: >> num-lanes: >> @@ -90,8 +91,19 @@ allOf: >> - if: >> properties: >> compatible: >> - enum: >> - - ti,j784s4-pcie-ep >> + items: >> + - const: ti,j721e-pcie-ep >> + then: >> + properties: >> + num-lanes: >> + minimum: 1 >> + maximum: 4 >> + >> + - if: >> + properties: >> + compatible: >> + items: >> + - const: ti,j784s4-pcie-ep > > Why? Previous code was correct. Though I used "patience diff", for some reason the addition of "ti,j721e-pcie-ep" in the check has been treated as the removal of "ti,j784s4-pcie-ep" first followed by adding the same later for generating the diff in this patch. The diff above is equivalent to the addition of: - if: properties: compatible: items: - const: ti,j721e-pcie-ep then: properties: num-lanes: minimum: 1 maximum: 4
On 17/01/2024 11:47, Siddharth Vadapalli wrote: > Hello Krzysztof, > > On 17/01/24 16:04, Krzysztof Kozlowski wrote: >> On 17/01/2024 11:25, Siddharth Vadapalli wrote: >>> The existing implementation for validating the "num-lanes" property >>> based on the compatible(s) doesn't enforce it. Fix it by updating the >>> checks to handle both single-compatible and multi-compatible cases. >>> >>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes") >>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings") >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>> --- >>> .../bindings/pci/ti,j721e-pci-ep.yaml | 26 ++++++++++++++----- >>> .../bindings/pci/ti,j721e-pci-host.yaml | 26 ++++++++++++++----- >>> 2 files changed, 38 insertions(+), 14 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>> index 97f2579ea908..278e0892f8ac 100644 >>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>> @@ -68,8 +68,9 @@ allOf: >>> - if: >>> properties: >>> compatible: >> >> Missing contains:, instead of your change. > > I did try the "contains" approach before determining that the implementation in > this patch is more suitable. Please consider the following: > > For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible > is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is > "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep". > > Therefore, the device-tree nodes for AM64 and J7200 look like: > > AM64: > compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep"; > ... > num-lanes = 1; > > J7200: > compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep"; > ... > num-lanes = 4; > > This implies that when the check for "num-lanes" is performed on the device-tree > node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the > AM64's "compatible: contains:" check will match the schema and it will check the > existing "num-lanes" being described as "const: 1" against the value in J7200's > PCIe node resulting in a warning. What warning? What did you put to contains? > Therefore, using "contains" will result in > errors if the check has to be performed for device-tree nodes with fallback > compatibles. The "items" based approach I have used in this patch ensures that > the schema matches *only* when both the primary and fallback compatible are > present in the device-tree node. Long message, but I don't understand it. Why this binding is different than all others which rely on contains? >>> + - if: >>> + properties: >>> + compatible: >>> + items: >>> + - const: ti,j784s4-pcie-ep >> >> Why? Previous code was correct. > > Though I used "patience diff", for some reason the addition of > "ti,j721e-pcie-ep" in the check has been treated as the removal of > "ti,j784s4-pcie-ep" first followed by adding the same later for generating the > diff in this patch. The diff above is equivalent to the addition of: No, why do you change existing code? It is correct. Best regards, Krzysztof
On 17/01/24 16:23, Krzysztof Kozlowski wrote: > On 17/01/2024 11:47, Siddharth Vadapalli wrote: >> Hello Krzysztof, >> >> On 17/01/24 16:04, Krzysztof Kozlowski wrote: >>> On 17/01/2024 11:25, Siddharth Vadapalli wrote: >>>> The existing implementation for validating the "num-lanes" property >>>> based on the compatible(s) doesn't enforce it. Fix it by updating the >>>> checks to handle both single-compatible and multi-compatible cases. >>>> >>>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes") >>>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings") >>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>> --- >>>> .../bindings/pci/ti,j721e-pci-ep.yaml | 26 ++++++++++++++----- >>>> .../bindings/pci/ti,j721e-pci-host.yaml | 26 ++++++++++++++----- >>>> 2 files changed, 38 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>>> index 97f2579ea908..278e0892f8ac 100644 >>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>>> @@ -68,8 +68,9 @@ allOf: >>>> - if: >>>> properties: >>>> compatible: >>> >>> Missing contains:, instead of your change. >> >> I did try the "contains" approach before determining that the implementation in >> this patch is more suitable. Please consider the following: >> >> For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible >> is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is >> "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep". >> >> Therefore, the device-tree nodes for AM64 and J7200 look like: >> >> AM64: >> compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep"; >> ... >> num-lanes = 1; >> >> J7200: >> compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep"; >> ... >> num-lanes = 4; >> >> This implies that when the check for "num-lanes" is performed on the device-tree >> node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the >> AM64's "compatible: contains:" check will match the schema and it will check the >> existing "num-lanes" being described as "const: 1" against the value in J7200's >> PCIe node resulting in a warning. > > What warning? What did you put to contains? The warning is: num-lanes: expected value is 1 which it has determined due to the presence of "ti,j721e-pcie-ep" in the first check which is only applicable to AM64. The shared fallback compatible here is responsible for incorrect checks when using "contains". Using "contains", the check for "num-lanes" with "const: 1" corresponding to AM64 ends up validating against the device-tree node for J7200 since the fallback compatible "ti,j721e-pcie-ep" is "contained" in the list of compatibles present in the device-tree node. That shouldn't be the case which is why "items" is used in this patch to get an exact match. > >> Therefore, using "contains" will result in >> errors if the check has to be performed for device-tree nodes with fallback >> compatibles. The "items" based approach I have used in this patch ensures that >> the schema matches *only* when both the primary and fallback compatible are >> present in the device-tree node. > > Long message, but I don't understand it. Why this binding is different > than all others which rely on contains? This binding is different because of the existence of a shared fallback compatible and a shared property being evaluated. In other bindings which use contains, either there isn't a shared fallback compatible, or the property which is present in device-tree nodes which have the shared fallback compatible isn't evaluated. In brief, with the existing device-tree, without any changes, adding "contains" will throw warnings due to the incorrect schema matching for validating the "num-lanes" property. > >>>> + - if: >>>> + properties: >>>> + compatible: >>>> + items: >>>> + - const: ti,j784s4-pcie-ep >>> >>> Why? Previous code was correct. >> >> Though I used "patience diff", for some reason the addition of >> "ti,j721e-pcie-ep" in the check has been treated as the removal of >> "ti,j784s4-pcie-ep" first followed by adding the same later for generating the >> diff in this patch. The diff above is equivalent to the addition of: > > No, why do you change existing code? It is correct. Either a "contains" or an "items" is required to evaluate the "num-lanes" property and neither of them are present in the existing code.
On 17/01/2024 12:11, Siddharth Vadapalli wrote: > > > On 17/01/24 16:23, Krzysztof Kozlowski wrote: >> On 17/01/2024 11:47, Siddharth Vadapalli wrote: >>> Hello Krzysztof, >>> >>> On 17/01/24 16:04, Krzysztof Kozlowski wrote: >>>> On 17/01/2024 11:25, Siddharth Vadapalli wrote: >>>>> The existing implementation for validating the "num-lanes" property >>>>> based on the compatible(s) doesn't enforce it. Fix it by updating the >>>>> checks to handle both single-compatible and multi-compatible cases. >>>>> >>>>> Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes") >>>>> Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings") >>>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >>>>> --- >>>>> .../bindings/pci/ti,j721e-pci-ep.yaml | 26 ++++++++++++++----- >>>>> .../bindings/pci/ti,j721e-pci-host.yaml | 26 ++++++++++++++----- >>>>> 2 files changed, 38 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>>>> index 97f2579ea908..278e0892f8ac 100644 >>>>> --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>>>> +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml >>>>> @@ -68,8 +68,9 @@ allOf: >>>>> - if: >>>>> properties: >>>>> compatible: >>>> >>>> Missing contains:, instead of your change. >>> >>> I did try the "contains" approach before determining that the implementation in >>> this patch is more suitable. Please consider the following: >>> >>> For AM64 SoC the primary compatible is "ti,am64-pcie-ep" and fallback compatible >>> is "ti,j721e-pcie-ep". For J7200 SoC the primary compatible is >>> "ti,j7200-pcie-ep" while the fallback compatible is again "ti,j721e-pcie-ep". >>> >>> Therefore, the device-tree nodes for AM64 and J7200 look like: >>> >>> AM64: >>> compatible = "ti,am64-pcie-ep", "ti,j721e-pcie-ep"; >>> ... >>> num-lanes = 1; >>> >>> J7200: >>> compatible = "ti,j7200-pcie-ep", "ti,j721e-pcie-ep"; >>> ... >>> num-lanes = 4; >>> >>> This implies that when the check for "num-lanes" is performed on the device-tree >>> node for PCIe in J7200, the fallback compatible of "ti,j721e-pcie-ep" within the >>> AM64's "compatible: contains:" check will match the schema and it will check the >>> existing "num-lanes" being described as "const: 1" against the value in J7200's >>> PCIe node resulting in a warning. >> >> What warning? What did you put to contains? > > The warning is: > num-lanes: expected value is 1 > which it has determined due to the presence of "ti,j721e-pcie-ep" in the first > check which is only applicable to AM64. The shared fallback compatible here is > responsible for incorrect checks when using "contains". > > Using "contains", the check for "num-lanes" with "const: 1" corresponding to > AM64 ends up validating against the device-tree node for J7200 since the > fallback compatible "ti,j721e-pcie-ep" is "contained" in the list of compatibles Why do you put fallback to contains? It does not make sense. You want to check for containing the device compatible. > present in the device-tree node. That shouldn't be the case which is why "items" > is used in this patch to get an exact match. > >> >>> Therefore, using "contains" will result in >>> errors if the check has to be performed for device-tree nodes with fallback >>> compatibles. The "items" based approach I have used in this patch ensures that >>> the schema matches *only* when both the primary and fallback compatible are >>> present in the device-tree node. >> >> Long message, but I don't understand it. Why this binding is different >> than all others which rely on contains? > > This binding is different because of the existence of a shared fallback Many other bindings are the same. > compatible and a shared property being evaluated. In other bindings which use > contains, either there isn't a shared fallback compatible, or the property which No, we do not talk about such bindings. We talk about fallbacks. Using contains for other cases is redundant, so why even bringing them up here? > is present in device-tree nodes which have the shared fallback compatible isn't > evaluated.> > In brief, with the existing device-tree, without any changes, adding "contains" > will throw warnings due to the incorrect schema matching for validating the > "num-lanes" property. ? What? > >> >>>>> + - if: >>>>> + properties: >>>>> + compatible: >>>>> + items: >>>>> + - const: ti,j784s4-pcie-ep >>>> >>>> Why? Previous code was correct. >>> >>> Though I used "patience diff", for some reason the addition of >>> "ti,j721e-pcie-ep" in the check has been treated as the removal of >>> "ti,j784s4-pcie-ep" first followed by adding the same later for generating the >>> diff in this patch. The diff above is equivalent to the addition of: >> >> No, why do you change existing code? It is correct. > > Either a "contains" or an "items" is required to evaluate the "num-lanes" > property and neither of them are present in the existing code. No, it's not. Your code is equivalent to old handling of j784s4. Contains is totally irrelevant here. NAK. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml index 97f2579ea908..278e0892f8ac 100644 --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-ep.yaml @@ -68,8 +68,9 @@ allOf: - if: properties: compatible: - enum: - - ti,am64-pcie-ep + items: + - const: ti,am64-pcie-ep + - const: ti,j721e-pcie-ep then: properties: num-lanes: @@ -78,9 +79,9 @@ allOf: - if: properties: compatible: - enum: - - ti,j7200-pcie-ep - - ti,j721e-pcie-ep + items: + - const: ti,j7200-pcie-ep + - const: ti,j721e-pcie-ep then: properties: num-lanes: @@ -90,8 +91,19 @@ allOf: - if: properties: compatible: - enum: - - ti,j784s4-pcie-ep + items: + - const: ti,j721e-pcie-ep + then: + properties: + num-lanes: + minimum: 1 + maximum: 4 + + - if: + properties: + compatible: + items: + - const: ti,j784s4-pcie-ep then: properties: num-lanes: diff --git a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml index b7a534cef24d..36bcc8cb7896 100644 --- a/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml +++ b/Documentation/devicetree/bindings/pci/ti,j721e-pci-host.yaml @@ -97,8 +97,9 @@ allOf: - if: properties: compatible: - enum: - - ti,am64-pcie-host + items: + - const: ti,am64-pcie-host + - const: ti,j721e-pcie-host then: properties: num-lanes: @@ -107,9 +108,9 @@ allOf: - if: properties: compatible: - enum: - - ti,j7200-pcie-host - - ti,j721e-pcie-host + items: + - const: ti,j7200-pcie-host + - const: ti,j721e-pcie-host then: properties: num-lanes: @@ -119,8 +120,19 @@ allOf: - if: properties: compatible: - enum: - - ti,j784s4-pcie-host + items: + - const: ti,j721e-pcie-host + then: + properties: + num-lanes: + minimum: 1 + maximum: 4 + + - if: + properties: + compatible: + items: + - const: ti,j784s4-pcie-host then: properties: num-lanes:
The existing implementation for validating the "num-lanes" property based on the compatible(s) doesn't enforce it. Fix it by updating the checks to handle both single-compatible and multi-compatible cases. Fixes: b3ba0f6e82cb ("dt-bindings: PCI: ti,j721e-pci-*: Add checks for num-lanes") Fixes: adc14d44d7cb ("dt-bindings: PCI: ti,j721e-pci-*: Add j784s4-pci-* compatible strings") Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- .../bindings/pci/ti,j721e-pci-ep.yaml | 26 ++++++++++++++----- .../bindings/pci/ti,j721e-pci-host.yaml | 26 ++++++++++++++----- 2 files changed, 38 insertions(+), 14 deletions(-)