[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3cecb092-2175-4e57-83d4-4507a902762a@kernel.org>
Date: Sat, 13 Jul 2024 11:53:43 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jim Quinlan <james.quinlan@...adcom.com>
Cc: linux-pci@...r.kernel.org, Nicolas Saenz Julienne <nsaenz@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Cyril Brulebois <kibi@...ian.org>, Stanimir Varbanov <svarbanov@...e.de>,
bcm-kernel-feedback-list@...adcom.com, jim2101024@...il.com,
Florian Fainelli <florian.fainelli@...adcom.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-rpi-kernel@...ts.infradead.org>,
"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE"
<linux-arm-kernel@...ts.infradead.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 01/12] dt-bindings: PCI: Add Broadcom STB 7712 SOC,
update maintainer
On 12/07/2024 22:13, Jim Quinlan wrote:
> On Sun, Jul 7, 2024 at 7:58 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>>
>> On 05/07/2024 22:02, Jim Quinlan wrote:
>>> On Thu, Jul 4, 2024 at 2:40 AM Krzysztof Kozlowski <krzk@...nel.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@...adcom.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@...e.de>
>>>>> + - Jim Quinlan <james.quinlan@...adcom.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.
> 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
Powered by blists - more mailing lists