Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/07/2024 22:02, Jim Quinlan wrote:
> On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On 03/07/2024 20:02, Jim Quinlan wrote:
>>> - Update maintainer; Nicolas hasn't been active and it
>>>   makes more sense to have a Broadcom maintainer
>>> - Add a driver compatible string for the new STB SOC 7712
>>
>> You meant device? Bindings are for hardware.
>>
>>> - Add two new resets for the 7712: "bridge", for the
>>>   the bridge between the PCIe core and the memory bus;
>>>   "swinit", the PCIe core reset.
>>> - Order the compatible strings alphabetically
>>> - Restructure the reset controllers so that the definitions
>>>   appear first before any rules that govern them.
>>
>> Please split cleanups from new device support.
>>
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 44 +++++++++++++++----
>>>  1 file changed, 36 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..a070f35d28d7 100644
>>> --- a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> @@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>  title: Brcmstb PCIe Host Controller
>>>
>>>  maintainers:
>>> -  - Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
>>> +  - Jim Quinlan <james.quinlan@broadcom.com>
>>>
>>>  properties:
>>>    compatible:
>>> @@ -16,11 +16,12 @@ properties:
>>>            - brcm,bcm2711-pcie # The Raspberry Pi 4
>>>            - brcm,bcm4908-pcie
>>>            - brcm,bcm7211-pcie # Broadcom STB version of RPi4
>>> -          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>>            - brcm,bcm7216-pcie # Broadcom 7216 Arm
>>> -          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>> +          - brcm,bcm7278-pcie # Broadcom 7278 Arm
>>>            - brcm,bcm7425-pcie # Broadcom 7425 MIPs
>>>            - brcm,bcm7435-pcie # Broadcom 7435 MIPs
>>> +          - brcm,bcm7445-pcie # Broadcom 7445 Arm
>>> +          - brcm,bcm7712-pcie # STB sibling SOC of Raspberry Pi 5
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -95,6 +96,20 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    items:
>>> +      - description: reset for phy calibration
>>> +      - description: reset for PCIe/CPU bus bridge
>>> +      - description: reset for soft PCIe core reset
>>> +      - description: reset for PERST# PCIe signal
>>
>> This won't work and I doubt you tested your code. You miss minItems.
> 
> I did test my code and there were no errors.  I perform the following test:
> 
> make ARCH=arm64 dt_binding_check DT_CHECKER_FLAGS=-m
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
> 
> Is this incorrect?

That's correct and you are right - it passes the checks. Recent dtschema
changed the logic behind this. I am not sure if the new approach will
stay, I would find explicit minItems here more obvious and readable, so:
resets:
  minItems: 1
  items:
    - .........
    - .........
    - .........
    - .........


> 
>>
>>> +
>>> +  reset-names:
>>> +    items:
>>> +      - const: rescal
>>> +      - const: bridge
>>> +      - const: swinit
>>> +      - const: perst
>>
>> This does not match what you have in conditional, so just keep min and
>> max Items here.
> 
> I'm not sure what you mean.  One chips uses a single reset, another
> chip uses a different single reset,
> and the third (7712) uses three of the four resets.

Your conditional in allOf:if:then has different order.

> 
> I was instructed to separate the descriptions from the rules, or at
> least that's what I thought I was asked.
>>
>>
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>> @@ -118,13 +133,10 @@ allOf:
>>>      then:
>>>        properties:
>>>          resets:
>>> -          items:
>>> -            - description: reset controller handling the PERST# signal
>>> -
>>> +          minItems: 1
>>
>> maxItems instead. Why three resets should be valid?
> 
> See above.  Note that I was just instructed to separate the rules from
> the descriptions.
> In doing so I placed all of the reset descripts on the top and then
> the rules below.
> There are four possible resets but no single chip uses all of them and
> three chips
> use one or three of them.
> 
> Please advise.

I don't understand that explanation. Why this particular variant works
with 1, 2, 3 or 4 resets in the same time?

Constraints are supposed to be precise / exact.

Best regards,
Krzysztof