Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for num-lanes

[Siddharth Vadapalli]

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@xxxxxx>
>>>> ---
>>>>  .../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.

-- 
Regards,
Siddharth.