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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/07/2024 21:54, 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.
> 
> Hello Krzysztof,
> 
> I should have replied to this before sending out V3.  Since your form
> letter says I did not address previous comments, I will address them
> here and now (your v2 review of the bindings commit).
> 
>>
>>> - 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.
> Okay.
>>
>>>
>>> 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.
>>
>>> +
>>> +  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 do not understand.  There are four possible resets, but any one chip
> uses only 0, 1, or 3 of them:
> 
>     CHIP            NUM_RESETS    NAMES
>     ====            ==========    =====
>     4908            1             perst
>     7216            1             rescal
>     7712            3             rescal, bridge, swinit
>     Other_Chips     0             -
> 
> Although I list four "reset-names", I have, in the rule for 7712,
> maxItems=3 because it only uses rescal, bridge, and swinit.  So I
> don't know what you mean when you say "this does not match what you
> have in your conditional".  AFAICT, they are not supposed to match.

One place says they have order A+B+C, other place says they have order
C+B+A or whatever other combination. Look at first element: A ! = C. So
they do not match.


> 
> 
>>
>>
>>> +
>>>  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?
> For the "4908" conditional, minItems==maxItems==1.  I do not
> understand your question "Why three resets should be valid" -- can you
> please elaborate?

Where do you have maxItems? I see only minItems.

> 
>>
>>
>>>          reset-names:
>>>            items:
>>>              - const: perst
>>> -
>>>        required:
>>>          - resets
>>>          - reset-names
>>> @@ -136,12 +148,28 @@ allOf:
>>>      then:
>>>        properties:
>>>          resets:
>>> +          minItems: 1
>>> +        reset-names:
>>>            items:
>>> -            - description: phandle pointing to the RESCAL reset controller
>>> +            - const: rescal
>>> +      required:
>>> +        - resets
>>> +        - reset-names
>>
>> Why?
> 
> I do not know what you are questioning.  The 7216 device uses one
> reset: the "rescal".  Again, maxItems==minItems==1.  Please see the
> summary note below.

You are breaking the ABI, so I am questioning. I don't see ABI break
explained in the commit msg.

> 
>>
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: brcm,bcm7712-pcie
>>> +    then:
>>> +      properties:
>>> +        resets:
>>> +          minItems: 3
>>
>> Again, you do not have 4 items here.
> 
> I do not want to have 4 items here; I want to have 3 for "rescal",
> "bridge," and "swinit".  In this case, maxItems==minItems==3.

Your schema does not define that.

> 
> Now , for V1 you requested that I define all resets at the top; I've
> done that and there are 4 of them.  But no chip uses all 4; each
> individual chip only uses 0, 1, or 3 resets.

I assumed they follow same order. If you have different order, the top
defines only widest constraints.

> 
> So there is no way that each chip's conditional rule can define
> minItems and maxItems to match the description list of 4 resets,
> unless you want me to undo your V1 request of describing the resets at
> the top level instead of describing them in the rules.
> 


Best regards,
Krzysztof