Message-ID: <90bdc95f-c4a7-40c8-aa55-e4460a4ce9dd@linaro.org>
Date: Wed, 17 Jan 2024 12:17:56 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Siddharth Vadapalli <s-vadapalli@...com>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kw@...ux.com,
 robh@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 vigneshr@...com, afd@...com, srk@...com
Subject: Re: [PATCH 1/3] dt-bindings: PCI: ti,j721e-pci-*: Fix check for
 num-lanes

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

Powered by blists - more mailing lists