linux-kernel - Re: [PATCH v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and add 7712 SoC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <90d1d4a5-5d47-4ad0-ab0f-d4f549cd4eec@kernel.org>
Date: Wed, 24 Jul 2024 10:05:37 +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 v4 01/12] dt-bindings: PCI: Cleanup of brcmstb YAML and
 add 7712 SoC

On 23/07/2024 20:44, Jim Quinlan wrote:
> On Wed, Jul 17, 2024 at 2:51 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>>
>> On 16/07/2024 23:31, Jim Quinlan wrote:
>>> o Change order of the compatible strings to be alphabetical
>>>
>>> o Describe resets/reset-names before using them in rules
>>>
>>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>> </form letter>
>>
>>> o Add minItems/maxItems where needed.
>>>
>>> o Change maintainer: Nicolas has not been active for a while.  It also
>>>   makes sense for a Broadcom employee to be the maintainer as many of the
>>>   details are privy to Broadcom.
>>>
>>> Signed-off-by: Jim Quinlan <james.quinlan@...adcom.com>
>>> ---
>>>  .../bindings/pci/brcm,stb-pcie.yaml           | 26 ++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml b/Documentation/devicetree/bindings/pci/brcm,stb-pcie.yaml
>>> index 11f8ea33240c..692f7ed7c98e 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,11 @@ 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
>>>
>>>    reg:
>>>      maxItems: 1
>>> @@ -95,6 +95,18 @@ properties:
>>>        minItems: 1
>>>        maxItems: 3
>>>
>>> +  resets:
>>> +    minItems: 1
>>> +    items:
>>> +      - description: reset for external PCIe PERST# signal # perst
>>> +      - description: reset for phy reset calibration       # rescal
>>> +
>>> +  reset-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: perst
>>> +      - const: rescal
>>
>> There are no devices with two resets. Anyway, this does not match one of
>> your variants which have first element as rescal.
> 
> 
> Hello Krzysztof,
> 
> At this commit there are two resets; the 4908 requires "perst" and the
> 7216 requires "rescal".   I now think what you are looking for is the
> top-level
> description of something like
> 
> resets:
>   maxItems: 1
>     oneOf:
>       - description: reset controller handling the PERST# signal
>       - description: phandle pointing to the RESCAL reset controller

Now tell me, what sort of new information comes with this description?
"Phandle"? It cannot be anything else. Redundant. "Pointing to"?
Redundant. "reset-controller"? Well, resets always point to reset
controller.

So what is the point of this description? Any point?

> 
> reset-names:
>   maxItems: 1
>     oneOf:
>       - const: perst
>       - const: rescal
> 
> I left out minItems because imItems==maxItems=1
> 
> Before I was giving both of them as the "potential candidates list"
> that will be used further on, but this is not how Yaml should be used.
> 
> Is the above in the right direction?

It's over complicated. First maxItems are redundant, because you define
number of items in items. Second, you have EXACTLY the same case as the
hardware for which I gave you bindings to use. I don't understand why
you insist on doing things differently, but you can. Take a look at many
other bindings how they achieve this - there are many, many examples.
But do not invent third or fourth method...

Best regards,
Krzysztof