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

[Krzysztof Kozlowski]

On 12/07/2024 22:13, Jim Quinlan wrote:
> On Sun, Jul 7, 2024 at 7:58 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>>
>> On 05/07/2024 22:02, Jim Quinlan wrote:
>>> On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@xxxxxxxxxx> 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@xxxxxxxxxxxx>
>>>>> ---
>>>>>  .../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@xxxxxxx>
>>>>> +  - Jim Quinlan <james.quinlan@xxxxxxxxxxxx>
>>>>>
>>>>>  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.
> Different order then what, and ordering by chip or by reset name?

Where is my comment? Comment is under reset-names.

> 
>>
>>>
>>> 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?
> 
> What do you mean in the "same time"?  The resets are just not present

Your schema says that you can have 1, 2, 3 or 4 resets.

> in most of our PCIe HW.  In two chips there is only 1 reset in the HW,
> and in the 7712 there are 3 resets in the HW.   You asked me to
> describe all of the resets first at the top level and I have done
> that.  But none of our chips ever use all four.
> 

Then express specific constraints in schema.

Best regards,
Krzysztof