Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC, update maintainer
From: Krzysztof Kozlowski
Date: Sat Jul 13 2024 - 05:52:51 EST
On 12/07/2024 21:54, 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.
>
> 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@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.
>>
>>> +
>>> + 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